llvm / llvm/llvm-project

HotColdSplitting shouldn't treat blocks as cold just because one path to them is cold

Open
#205,215 1 comment 0 reactions 0 assignees View on GitHub
ipo
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

In `llvm/lib/Transforms/IPO/HotColdSplitting.cpp`, in the function `analyzeProfMetadata`, if a conditional branch has a low probability of jumping to a given destination block, that destination block is marked cold. The problem is that this happens even if the function has other paths to the same block that are not low-probability.

Test case [(Godbolt link)](https://gcc.godbolt.org/z/We7zrer7r)

```c
void some_func(int a, int b) {
if (a) {
if (__builtin_expect(b, 1)) {
puts("this is likely");
}
puts("this should be neutral");
}
}
```

When compiled with hot-cold splitting enabled, LLVM splits out the entire body of the `if (a)` into a cold function, even though it's clearly not cold.

The relevant part of the control flow is:
```
┌───────────────────────────────┐
│ │
│ if (__builtin_expect(b, 1)) │
│ │
└───────────────────────────────┘
│ ╲
│ likely
│ ╲
│ ▼
│ ┌───────────────────────────┐
│ │ │
unlikely │ puts("this is likely"); │
│ │ │
│ └───────────────────────────┘
│ ╱
│ unconditional
│ ╱
▼ ▼
┌───────────────────────────────────┐
│ │
│ puts("this should be neutral"); │
│ │
└───────────────────────────────────┘
```

The bottom block can be reached through an unlikely branch, so `analyzeProfMetadata` incorrectly marks it as cold, even though it can also be reached from an unconditional branch following a likely branch.

The remaining logic is correct: `OutliningRegion::create` goes on to mark the top and middle blocks as cold, because they are now post-dominated by a cold block, and then all three blocks are extracted together.

### Fix

It sounds like the correct thing to do would be to rely on `BlockFrequencyInfo`, which essentially propagates probabilities through the control flow graph, rather than raw branch weights. In fact, `HotColdSplitting` already has a path that uses `BlockFrequencyInfo` and in this path the culprit function (`analyzeProfMetadata`) is not even used. But this path also depends on `ProfileSummaryInfo` being present, and I guess it usually isn't. There are already some TODO comments suggesting this should be improved. In theory `BlockFrequencyInfo` could be used without `ProfileSummaryInfo`, but it sounds like calculating the `BlockFrequencyInfo` is itself expensive, judging by commit cde65c0face16507ceb7e0d6f5d36f6f1b9cc04d.

(That commit made `HotColdSplitting` skip calculating `BlockFrequencyInfo` if `ProfileSummaryInfo` was missing. It was a non-functional change, because the current implementation doesn't actually use the `BlockFrequencyInfo` in that case. But if we wanted to change `HotColdSplitting` to always use `BlockFrequencyInfo`, we'd have to forego the performance benefit, which is described as "reduc[ing] compile-time significantly".)

Perhaps there is a better time to run `HotColdSplitting` when the relevant info would already be available?

Well, I don't really know what I'm talking about.

A more targeted fix would be to change `analyzeProfMetadata` to only mark a block cold if *every* predecessor is either low-probability or from a cold block itself. This would probably be fine, I don't know how much it would increase the number of false negatives. Currently, `HotColdSplitting` will fail to realize a block is cold if it's post-dominated by a low-probability branch (but not the direct successor of one). There is post-domination analysis in `OutliningRegion::create`, but that is only called after `analyzeProfMetadata` has already decided a block is cold, and the analysis is only used locally to make that particular outlining region and isn't saved for the rest of the `HotColdSplitting` pass (except in the special case where the entire function is cold). To be clear, these false negatives already exist. But a block that would otherwise be a false negative (i.e. it ought to be cold but `analyzeProfMetadata` doesn't realize it) might be 'rescued', unsoundly, if there happens to be another branch that's directly low-probability. Fixing the unsoundness would prevent the rescue. I have no idea how common (or not) this situation is.

Contributor guide

Open the contributing guide

Research direction

Start in llvm/lib/Transforms/IPO/HotColdSplitting.cpp at analyzeProfMetadata and reproduce the issue with the linked Godbolt test case. Read the existing BlockFrequencyInfo/ProfileSummaryInfo path and OutliningRegion::create before deciding how predecessor probabilities should be handled. Done means a block reached by another non-low-probability path is not incorrectly marked cold, with the reported example no longer extracting the neutral block.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.