Details

Reviewers

davidxl
mtrofin
wmi
vsk
aartbik

Commits

rG10f2a0d662d8: Use uint64_t for branch weights instead of uint32_t
rGe5766f25c62c: Use uint64_t for branch weights instead of uint32_t

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

Repository: rG LLVM Github Monorepo

Unit TestsFailed
View All

	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

aeubanks created this revision.Sep 30 2020, 1:02 PM

Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2020, 1:02 PM

Herald added subscribers: llvm-commits, wenlei, dexonsmith and 4 others. · View Herald Transcript

aeubanks requested review of this revision.Sep 30 2020, 1:02 PM

aeubanks added reviewers: davidxl, mtrofin, wmi.Sep 30 2020, 1:04 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.

Harbormaster completed remote builds in B73566: Diff 295383.Sep 30 2020, 1:13 PM

In D88609#2304497, @davidxl wrote:

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.

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.

Harbormaster completed remote builds in B75918: Diff 299766.Oct 21 2020, 11:57 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.

lgtm

This revision is now accepted and ready to land.Oct 21 2020, 12:45 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

Harbormaster completed remote builds in B76270: Diff 300447.Oct 23 2020, 6:56 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

Harbormaster completed remote builds in B76457: Diff 300764.Oct 26 2020, 1:38 PM

This revision was landed with ongoing or failed builds.Oct 26 2020, 8:35 PM

Closed by commit rGe5766f25c62c: Use uint64_t for branch weights instead of uint32_t (authored by aeubanks). · Explain Why

This revision was automatically updated to reflect the committed changes.

aeubanks added a commit: rGe5766f25c62c: Use uint64_t for branch weights instead of uint32_t.

kiranchandramohan mentioned this in rG0fc1aa22ee6a: Updating llvm.mlir test to match recent IR change.Oct 27 2020, 1:18 AM

Updated mlir test also to include this change.
https://reviews.llvm.org/rG0fc1aa22ee6ac337a5d51fa5666c9cd61da61b07

• thakis added a reverting change: rG2a4e704c92e8: Revert "Use uint64_t for branch weights instead of uint32_t".Oct 27 2020, 6:26 AM

@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

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

Herald added a reviewer: aartbik. · View Herald TranscriptOct 28 2020, 1:16 PM

Herald added a project: Restricted Project. · View Herald Transcript

Herald added subscribers: rdzhabarov, tatianashp, msifontes and 15 others. · View Herald Transcript

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?

Harbormaster completed remote builds in B76802: Diff 301409.Oct 28 2020, 4:25 PM

This revision was landed with ongoing or failed builds.Oct 30 2020, 10:03 AM

Closed by commit rG10f2a0d662d8: Use uint64_t for branch weights instead of uint32_t (authored by aeubanks). · Explain Why

This revision was automatically updated to reflect the committed changes.

aeubanks added a commit: rG10f2a0d662d8: Use uint64_t for branch weights instead of uint32_t.

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.

aeubanks added a reverting change: rG5c31b8b94fdd: Revert "Use uint64_t for branch weights instead of uint32_t".Oct 31 2020, 12:26 AM

aeubanks mentioned this in D90539: Make CallInst::updateProfWeight emit i32 weights instead of i64.Nov 3 2020, 7:24 PM

Use uint64_t for branch weights instead of uint32_tClosedPublicActions

Details

Diff Detail

Unit TestsFailedView All

Event Timeline

Revision ContentsChangeset List

Diff 300447

clang/lib/CodeGen/CodeGenPGO.cpp

clang/test/CodeGen/catch-undef-behavior.c

clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp

clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp

clang/test/CodeGenCXX/attr-likelihood-switch-branch-weights.cpp

clang/test/CodeGenCXX/microsoft-abi-thread-safe-statics.cpp

clang/test/CodeGenCXX/profile-remap.cpp

clang/test/CodeGenCXX/static-initializer-branch-weights.cpp

clang/test/Profile/c-captured.c

clang/test/Profile/c-counter-overflows.c

clang/test/Profile/c-general.c

clang/test/Profile/cxx-class.cpp

clang/test/Profile/cxx-lambda.cpp

clang/test/Profile/cxx-rangefor.cpp

clang/test/Profile/cxx-templates.cpp

clang/test/Profile/cxx-throws.cpp

clang/test/Profile/gcc-flag-compatibility.c

clang/test/Profile/objc-general.m

llvm/include/llvm/IR/Instructions.h

llvm/include/llvm/IR/MDBuilder.h

llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h

llvm/include/llvm/Transforms/Utils/MisExpect.h

llvm/lib/Analysis/BranchProbabilityInfo.cpp

llvm/lib/IR/Instructions.cpp

llvm/lib/IR/MDBuilder.cpp

llvm/lib/Transforms/IPO/SampleProfile.cpp

llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

llvm/lib/Transforms/Scalar/JumpThreading.cpp

llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

llvm/lib/Transforms/Utils/CodeExtractor.cpp

llvm/lib/Transforms/Utils/Local.cpp

llvm/lib/Transforms/Utils/MisExpect.cpp

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

llvm/test/Analysis/BranchProbabilityInfo/basic.ll

llvm/test/Analysis/BranchProbabilityInfo/basic_i32.ll

llvm/test/Instrumentation/AddressSanitizer/basic.ll

llvm/test/Transforms/CodeExtractor/MultipleExitBranchProb.ll

llvm/test/Transforms/CorrelatedValuePropagation/profmd.ll

llvm/test/Transforms/JumpThreading/threading_prof1.ll

llvm/test/Transforms/JumpThreading/threading_prof2.ll

llvm/test/Transforms/JumpThreading/update-edge-weight.ll

llvm/test/Transforms/LoopUnroll/peel-loop-pgo-deopt.ll

llvm/test/Transforms/LoopUnroll/peel-loop-pgo.ll

llvm/test/Transforms/LoopUnroll/runtime-loop-branchweight.ll

llvm/test/Transforms/LoopVectorize/check-prof-info.ll

llvm/test/Transforms/LoopVectorize/tripcount.ll

llvm/test/Transforms/LowerExpectIntrinsic/basic.ll

llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll

llvm/test/Transforms/LowerExpectIntrinsic/expect_nonboolean.ll

llvm/test/Transforms/LowerExpectIntrinsic/phi_merge.ll

llvm/test/Transforms/LowerExpectIntrinsic/phi_or.ll

llvm/test/Transforms/LowerExpectIntrinsic/phi_tern.ll

llvm/test/Transforms/PGOProfile/branch1.ll

llvm/test/Transforms/PGOProfile/branch2.ll

llvm/test/Transforms/PGOProfile/chr.ll

llvm/test/Transforms/PGOProfile/criticaledge.ll

llvm/test/Transforms/PGOProfile/cspgo_profile_summary.ll

llvm/test/Transforms/PGOProfile/fix_entry_count.ll

llvm/test/Transforms/PGOProfile/icp_covariant_call_return.ll

llvm/test/Transforms/PGOProfile/icp_covariant_invoke_return.ll

llvm/test/Transforms/PGOProfile/icp_invoke.ll

llvm/test/Transforms/PGOProfile/icp_vararg.ll

llvm/test/Transforms/PGOProfile/indirect_call_promotion.ll

llvm/test/Transforms/PGOProfile/indirect_call_promotion_musttail.ll

llvm/test/Transforms/PGOProfile/indirect_call_promotion_vla.ll

llvm/test/Transforms/PGOProfile/indirectbr.ll

llvm/test/Transforms/PGOProfile/instr_entry_bb.ll

llvm/test/Transforms/PGOProfile/landingpad.ll

llvm/test/Transforms/PGOProfile/loop1.ll

llvm/test/Transforms/PGOProfile/loop2.ll

llvm/test/Transforms/PGOProfile/memop_size_opt.ll

llvm/test/Transforms/PGOProfile/multiple_hash_profile.ll

Use uint64_t for branch weights instead of uint32_t
ClosedPublic
Actions

Unit TestsFailed
View All

Revision Contents
Changeset List