Page MenuHomePhabricator

Make CallInst::updateProfWeight emit i32 weights instead of i64
ClosedPublic

Authored by aeubanks on Oct 31 2020, 12:43 PM.

Details

Summary

Typically branch_weights are i32, not i64.
This fixes entry_counts_cold.ll under NPM.

Diff Detail

Event Timeline

aeubanks created this revision.Oct 31 2020, 12:43 PM
aeubanks requested review of this revision.Oct 31 2020, 12:43 PM

Would we rather want them to be i64, or is there a fundamental reason they should be i32?

Ideally they'd be i64, see https://reviews.llvm.org/D88609 where I tried doing that, but it was reverted multiple times due to uint64_t overflows and I gave up trying to fix the various issues. There are multiple instances of using uint64_t to do uint32_t arithmetic and checking for overflows, and I eventually got annoyed with all those and gave up.

Ideally they'd be i64, see https://reviews.llvm.org/D88609 where I tried doing that, but it was reverted multiple times due to uint64_t overflows and I gave up trying to fix the various issues. There are multiple instances of using uint64_t to do uint32_t arithmetic and checking for overflows, and I eventually got annoyed with all those and gave up.

Ah... iiuc, with 32 bit, overflow can be detected more easily?

Yeah, just do 32-bit arithmetic with uint64_t and you can see if the final result overflows. One example is here: https://github.com/llvm/llvm-project/blob/7ba3293691beb9a2c6ea4a81064c24580afe5816/llvm/lib/Analysis/BranchProbabilityInfo.cpp#L486

The main issue is that this is done in multiple places around LLVM.

Ping
We can always fix everything to use i64 in a follow-up change, but for now I'd like to fix entry_counts_cold.ll under the NPM.

Ping
We can always fix everything to use i64 in a follow-up change, but for now I'd like to fix entry_counts_cold.ll under the NPM.

I agree this should be fixed separately from updating the world (that could be before or after this patch). I'm also a bit skeptical that it's valuable to have more than i32 for branch weights.

llvm/lib/IR/Instructions.cpp
563–565

I think you need getLimitedValue(UINT32_MAX) here. Can you add a test that covers that as well?

aeubanks updated this revision to Diff 305294.Nov 13 2020, 7:09 PM

getLimitedValue(UINT32_MAX) and add test

asbirlea accepted this revision.Nov 24 2020, 4:36 PM

Unblocking the fixing of the tests under NPM. Additional changes can be done as follow ups.

This revision is now accepted and ready to land.Nov 24 2020, 4:36 PM
This revision was landed with ongoing or failed builds.Nov 24 2020, 6:28 PM
This revision was automatically updated to reflect the committed changes.