Page MenuHomePhabricator

Use uint64_t for branch weights instead of uint32_t
ClosedPublic

Authored by aeubanks on Sep 30 2020, 1:02 PM.

Details

Summary

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.

Diff Detail

Event Timeline

aeubanks created this revision.Sep 30 2020, 1:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2020, 1:02 PM
aeubanks requested review of this revision.Sep 30 2020, 1:02 PM
davidxl added a reviewer: vsk.Sep 30 2020, 1:12 PM

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

Perhaps add a new test to demonstrate it (that i32 profile weight still works)?

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.

aeubanks updated this revision to Diff 299766.Oct 21 2020, 11:41 AM

add test for i32 and i64

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.

vsk added a comment.Oct 21 2020, 11:59 AM

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.

This revision is now accepted and ready to land.Oct 21 2020, 12:45 PM
aeubanks updated this revision to Diff 300447.Oct 23 2020, 6:10 PM

check-clang revealed more failures, had to modify some clang tests and BranchProbabilityInfo.cpp

Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2020, 6:10 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aeubanks updated this revision to Diff 300764.Oct 26 2020, 12:51 PM

fix more tests found by running check-all on a Linux machine

Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2020, 12:51 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
This revision was landed with ongoing or failed builds.Oct 26 2020, 8:35 PM
This revision was automatically updated to reflect the committed changes.

@thakis Can you revert the testcase fix in MLIR also which was necessary after this patch? https://reviews.llvm.org/rG0fc1aa22ee6ac337a5d51fa5666c9cd61da61b07

aeubanks reopened this revision.Oct 28 2020, 1:16 PM
This revision is now accepted and ready to land.Oct 28 2020, 1:16 PM
aeubanks updated this revision to Diff 301409.Oct 28 2020, 1:16 PM

scale down weights by UINT32_MAX when sum of branch weights overflows uint64_t

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?

This revision was landed with ongoing or failed builds.Oct 30 2020, 10:03 AM
This revision was automatically updated to reflect the committed changes.
dmajor added a subscriber: dmajor.Oct 30 2020, 6:35 PM

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.