Switch to use cold threshold from profile summary for cold context merging and trimming, instead of relying on hard coded values. Minor refactoring included for switch names, etc.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
2,520 ms | x64 debian > libarcher.races::lock-unrelated.c |
Event Timeline
llvm/tools/llvm-profgen/ProfileGenerator.cpp | ||
---|---|---|
206 | We need profile name otherwise UseContextLessSummary won't work as the merge of context profile needs name key. |
llvm/tools/llvm-profgen/ProfileGenerator.cpp | ||
---|---|---|
47 | I'm wondering if the original switches can be moved into a common place, such as ProfileSummaryBuilder.cpp, so that they can be reused here. Some code in CSProfileGenerator::computeSummaryAndThreshold may also be sharable. |
llvm/tools/llvm-profgen/ProfileGenerator.cpp | ||
---|---|---|
399–408 | Similar as Hongtao's question. Can ProfileSummaryInfo::computeThresholds be used to set the hot/cold threshold? |
llvm/tools/llvm-profgen/ProfileGenerator.cpp | ||
---|---|---|
47 | That would be nice, however for normal path of ProfileSummaryInfo, the actual summary is retrieved from module metadata which is read from binary profile directly instead of being built from ProfileSummaryBuilder on the spot. I think it probably makes sense to just use ProfileSummaryInfo here, as Wei suggested. However, looks like the switches from ProfileSummaryInfo.cpp doesn't get exposed that way.. I guess that's because ProfileData is linked into llvm-profogen, instead of being part of the tool? | |
399–408 | Sounds good, changed to use PSI directly here, so we don't need keep thresholds separately. |
Using PSI looks good, thanks. Can you please add a test for -csprof-merge-cold-context? Otherwise lgtm.
llvm/test/tools/llvm-profgen/merge-cold-profile.test | ||
---|---|---|
54 | How about we add: ; CHECK-UNMERGED-NOT: [fa] ; CHECK-UNMERGED-NOT: [fb] to make sure the merging not happen. | |
llvm/tools/llvm-profgen/ProfileGenerator.cpp | ||
239 | There is two places to call this function: for probe and non-probe. |
llvm/test/tools/llvm-profgen/merge-cold-profile.test | ||
---|---|---|
54 | Sounds good, added. | |
llvm/tools/llvm-profgen/ProfileGenerator.cpp | ||
239 | I think it doesn't belong to writing, though moving there may save one call from the code. The thresholds will be needed for estimating inlining, which I will send up next, so I'd like to keep it this way for now if it's ok, and try to refactor in the upcoming change. |
LGTM, thanks!
llvm/tools/llvm-profgen/ProfileGenerator.cpp | ||
---|---|---|
239 | Sounds good, thanks for the clarification. |
How about we add:
to make sure the merging not happen.