[JumpThreading] Use [BB->SuccIndx] to get probability when updating BB info. #134585
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…B info. In case the same src BB targets to the same dest BB in different conditions/edges, such as switch-cases, we should use prob[SrcBB->SuccIndx] instead of prob[SrcBB->DstBB].
|
@llvm/pr-subscribers-llvm-transforms Author: None (tianleliu) ChangesIn case the same src BB targets to the same dest BB in different conditions/edges, such as switch-cases, we should use prob[SrcBB->SuccIndx] instead of prob[SrcBB->DstBB] to get probability. Full diff: https://github.com/llvm/llvm-project/pull/134585.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
index 18d5f201413c8..5c112a429c6bc 100644
--- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
@@ -2530,17 +2530,16 @@ void JumpThreadingPass::updateBlockFreqAndEdgeWeight(BasicBlock *PredBB,
// frequency of BB.
auto BBOrigFreq = BFI->getBlockFreq(BB);
auto NewBBFreq = BFI->getBlockFreq(NewBB);
- auto BB2SuccBBFreq = BBOrigFreq * BPI->getEdgeProbability(BB, SuccBB);
auto BBNewFreq = BBOrigFreq - NewBBFreq;
BFI->setBlockFreq(BB, BBNewFreq);
// Collect updated outgoing edges' frequencies from BB and use them to update
// edge probabilities.
SmallVector<uint64_t, 4> BBSuccFreq;
- for (BasicBlock *Succ : successors(BB)) {
- auto SuccFreq = (Succ == SuccBB)
- ? BB2SuccBBFreq - NewBBFreq
- : BBOrigFreq * BPI->getEdgeProbability(BB, Succ);
+ for (succ_iterator I = succ_begin(BB), E = succ_end(BB); I != E; ++I) {
+ auto BB2SuccBBFreq =
+ BBOrigFreq * BPI->getEdgeProbability(BB, I.getSuccessorIndex());
+ auto SuccFreq = (*I == SuccBB) ? BB2SuccBBFreq - NewBBFreq : BB2SuccBBFreq;
BBSuccFreq.push_back(SuccFreq.getFrequency());
}
diff --git a/llvm/test/Transforms/JumpThreading/thread-prob-8.ll b/llvm/test/Transforms/JumpThreading/thread-prob-8.ll
new file mode 100644
index 0000000000000..0952b87a35429
--- /dev/null
+++ b/llvm/test/Transforms/JumpThreading/thread-prob-8.ll
@@ -0,0 +1,41 @@
+; RUN: opt -debug-only=branch-prob -passes=jump-threading -S %s 2>&1 | FileCheck %s
+; REQUIRES: asserts
+
+; Make sure that edges' probabilities would not accumulate if they are
+; the same target BB.
+; Edge L0 -> 2 and L0 -> 3 's targets are both L2, but their respective
+; probability should not be L0 -> L2, because prob[L0->L2] equls to
+; prob[L0->2] + prob[L0->3]
+
+; CHECK: Computing probabilities for entry
+; CHECK: eraseBlock L0
+; CHECK-NOT: set edge L0 -> 0 successor probability to 0x12492492 / 0x80000000 = 14.29%
+; CHECK-NOT: set edge L0 -> 1 successor probability to 0x24924925 / 0x80000000 = 28.57%
+; CHECK-NOT: set edge L0 -> 2 successor probability to 0x24924925 / 0x80000000 = 28.57%
+; CHECK-NOT: set edge L0 -> 3 successor probability to 0x24924925 / 0x80000000 = 28.57%
+; CHECK: set edge L0 -> 0 successor probability to 0x1999999a / 0x80000000 = 20.00%
+; CHECK: set edge L0 -> 1 successor probability to 0x33333333 / 0x80000000 = 40.00%
+; CHECK: set edge L0 -> 2 successor probability to 0x1999999a / 0x80000000 = 20.00%
+; CHECK: set edge L0 -> 3 successor probability to 0x1999999a / 0x80000000 = 20.00%
+
+define void @test_switch(i1 %cond, i8 %value) nounwind {
+entry:
+ br i1 %cond, label %L0, label %L4
+L0:
+ %expr = select i1 %cond, i8 1, i8 %value
+ switch i8 %expr, label %L3 [
+ i8 1, label %L1
+ i8 2, label %L2
+ i8 3, label %L2
+ ], !prof !0
+
+L1:
+ ret void
+L2:
+ ret void
+L3:
+ ret void
+L4:
+ br label %L0
+}
+!0 = !{!"branch_weights", i32 1, i32 7, i32 1, i32 1}
|
nikic
reviewed
Apr 7, 2025
There was a problem hiding this comment.
Could you please test that the resulting branch weights in the IR are correct, instead of checking debug dumps?
There was a problem hiding this comment.
Hi Nikic, thanks for your review. I have added branch weight checking in test file.
I think dumping branch probability is helpful to understand what the patch changes. So I prefer to remain it?
There was a problem hiding this comment.
It limits the test to debug only, which is not great, but I agree that the output is clearer.
nikic
approved these changes
Apr 8, 2025
| ; CHECK-NOT: set edge L0 -> 0 successor probability to 0x12492492 / 0x80000000 = 14.29% | ||
| ; CHECK-NOT: set edge L0 -> 1 successor probability to 0x24924925 / 0x80000000 = 28.57% | ||
| ; CHECK-NOT: set edge L0 -> 2 successor probability to 0x24924925 / 0x80000000 = 28.57% | ||
| ; CHECK-NOT: set edge L0 -> 3 successor probability to 0x24924925 / 0x80000000 = 28.57% |
There was a problem hiding this comment.
I don't think these CHECK-NOTs are useful if you already CHECK the corresponding lines.
There was a problem hiding this comment.
It limits the test to debug only, which is not great, but I agree that the output is clearer.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/133/builds/14139 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In case the same src BB targets to the same dest BB in different conditions/edges, such as switch-cases, we should use prob[SrcBB->SuccIndx] instead of prob[SrcBB->DstBB] to get probability.