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
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.