HotColdSplitting shouldn't treat blocks as cold just because one path to them is cold
- 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
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