CallInst::updateProfWeight() creates branch_weights with i64 instead of i32.
To be more consistent everywhere and remove lots of casts from uint64_t
to uint32_t, use i64 for branch_weights.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Time | Test | |
---|---|---|
560 ms | linux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp Script:
--
: 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp
| |
80 ms | linux > LLVM.Transforms/SampleProfile::profile-format-compress.ll Script:
--
: 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/profile-format-compress.ll -sample-profile -sample-profile-file=/mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/Inputs/inline.prof -S | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/profile-format-compress.ll
| |
90 ms | linux > MLIR.Target::llvmir.mlir Script:
--
: 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/mlir-translate -mlir-to-llvmir -split-input-file /mnt/disks/ssd0/agent/llvm-project/mlir/test/Target/llvmir.mlir | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/mlir/test/Target/llvmir.mlir
| |
400 ms | linux > Profile-x86_64.Linux::comdat_rename.test Script:
--
: 'RUN: at line 1'; rm -fr /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/profile/Profile-x86_64/Linux/Output/comdat_rename.test.tmp.prof
| |
150 ms | linux > Profile-x86_64.Linux::instrprof-basic.c Script:
--
: 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -m64 -ldl -fprofile-instr-generate -fdata-sections -ffunction-sections -fuse-ld=gold -Wl,--gc-sections -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/profile/Profile-x86_64/Linux/Output/instrprof-basic.c.tmp -O3 /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/profile/Linux/instrprof-basic.c
| |
View Full Test Results (20 Failed) |
Event Timeline
I like the direction of the patch. There is one concern -- it makes the IR not backward compatible which may affect some users. +vsk.
I'm not too familiar with this, but it looks like weights are read via C->getValue().getZExtValue(), which should work with i32 and i64.
Maybe some version of LLVM without this commit that reads IR generated by a later version of LLVM with this commit will incorrectly read weights larger than i32, but not sure if that's a concern or not.
There are actually lots of tests I didn't touch that still have i32 branch_weights, for example llvm/test/Transforms/SampleProfile/remap.ll has !9 = !{!"branch_weights", i32 90, i32 10} and still properly prints out the same branch probabilities with opt -passes='print<branch-prob>'. Is that good enough?
Then why modifying existing tests at all? How about add an explicit test using i64 type?
The test modifications are because now when a pass creates a branch_weight, the type is now i64, so the CHECK lines need to be updated in those cases. Otherwise just reading an existing branch_weight is ok.
Basically I ran check-llvm, then in the failing tests I replaced all branch_weights with i64 values. Tests that didn't explicitly check for i32 branch_weights (which is most tests) still passed.
I changed llvm/test/Analysis/BranchProbabilityInfo/basic.ll to use i64 branch_weights and made llvm/test/Analysis/BranchProbabilityInfo/basic_i32.ll which uses i32 branch_weights.
To my knowledge, we're not relying on the fact that branch_weights are 32-bit for bitcode compatibility purposes, or for PGO. We do support using older .profdata files with newer builds, but (afaict) this change to branch_weights does not affect that.
check-clang revealed more failures, had to modify some clang tests and BranchProbabilityInfo.cpp
Updated mlir test also to include this change.
https://reviews.llvm.org/rG0fc1aa22ee6ac337a5d51fa5666c9cd61da61b07
@thakis Can you revert the testcase fix in MLIR also which was necessary after this patch? https://reviews.llvm.org/rG0fc1aa22ee6ac337a5d51fa5666c9cd61da61b07
This was reverted due to crashes caused by adding branch weights overflowing uint64_t. Now in BranchProbabilityInfo.cpp, if the sum of weights overflows, scale down all weights by UINT32_MAX (See ScaleWeights()). Is that ok?
After 10f2a0d662d8 our self-host PGO builds are hitting:
llvm::BranchProbability::getBranchProbability(uint64_t, uint64_t): Assertion `Numerator <= Denominator && "Probability cannot be bigger than 1!"' failed.
https://treeherder.mozilla.org/logviewer?job_id=320324631&repo=try&lineNumber=52597 has the log as well as the .cpp/.sh reproducers, but I assume you'd also need the merged.profdata file, and our bots haven't been taught to save those. I can try to add that ability next week if needed.
The stack trace helped me come up with my own repro, reverting.
It's more uint64_t overflow issues. This might be the reason branch_weights were initially i32 with casts in a bunch of places around LLVM. Not sure if I should try and fix all usages or just keep branch_weights as i32.