Skip to content

Conversation

mtrofin
Copy link
Member

@mtrofin mtrofin commented Aug 19, 2025

There’s a pattern where a branch is conditioned on a conjunction or disjunction that ends up being modeled as a select​ where the first operand is set to true​ or the second to false​. If the branch has known branch weights, they can be copied to the select​. This is worth doing in case later the select​ gets transformed to something else (i.e. if we know the profile, we should propagate it).

Issue #147390

@mtrofin mtrofin requested a review from david-xl August 19, 2025 21:44
@mtrofin mtrofin marked this pull request as ready for review August 19, 2025 21:44
@llvmbot
Copy link
Member

llvmbot commented Aug 19, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Mircea Trofin (mtrofin)

Changes

There’s a pattern where a branch is conditioned on a select​ where the first operand is set to true​. The branch has known branch weights, and they can be copied to the select​. This is worth doing in case later the select​ gets transformed to something else (i.e. if we know the profile, we should propagate it).

Issue #147390


Full diff: https://github.com/llvm/llvm-project/pull/154426.diff

3 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/SimplifyCFG.cpp (+10)
  • (modified) llvm/test/Transforms/SimplifyCFG/branch-fold.ll (+13-5)
  • (modified) llvm/test/Transforms/SimplifyCFG/preserve-branchweights.ll (+17-17)
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 055e8cadaab76..bd25f14ec85d0 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -203,6 +203,8 @@ static cl::opt<unsigned> MaxJumpThreadingLiveBlocks(
     cl::desc("Limit number of blocks a define in a threaded block is allowed "
              "to be live in"));
 
+extern cl::opt<bool> ProfcheckDisableMetadataFixes;
+
 STATISTIC(NumBitMaps, "Number of switch instructions turned into bitmaps");
 STATISTIC(NumLinearMaps,
           "Number of switch instructions turned into linear mapping");
@@ -4772,6 +4774,14 @@ static bool SimplifyCondBranchToCondBranch(BranchInst *PBI, BranchInst *BI,
     fitWeights(NewWeights);
 
     setBranchWeights(PBI, NewWeights[0], NewWeights[1], /*IsExpected=*/false);
+    // Cond may be a select instruction with the first operand set to "true".
+    if (!ProfcheckDisableMetadataFixes)
+      if (auto *SI = dyn_cast<SelectInst>(Cond)) {
+        assert(isa<ConstantInt>(SI->getTrueValue()) &&
+               (dyn_cast<ConstantInt>(SI->getTrueValue())->isOne()));
+        setBranchWeights(SI, NewWeights[0], NewWeights[1],
+                         /*IsExpected=*/false);
+      }
   }
 
   // OtherDest may have phi nodes.  If so, add an entry from PBI's
diff --git a/llvm/test/Transforms/SimplifyCFG/branch-fold.ll b/llvm/test/Transforms/SimplifyCFG/branch-fold.ll
index 2f5fb4f33013d..b51398dd6222d 100644
--- a/llvm/test/Transforms/SimplifyCFG/branch-fold.ll
+++ b/llvm/test/Transforms/SimplifyCFG/branch-fold.ll
@@ -1,12 +1,12 @@
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals
 ; RUN: opt < %s -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S | FileCheck %s
 
 define void @test(ptr %P, ptr %Q, i1 %A, i1 %B) {
 ; CHECK-LABEL: @test(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[A_NOT:%.*]] = xor i1 [[A:%.*]], true
-; CHECK-NEXT:    [[BRMERGE:%.*]] = select i1 [[A_NOT]], i1 true, i1 [[B:%.*]]
-; CHECK-NEXT:    br i1 [[BRMERGE]], label [[B:%.*]], label [[COMMON_RET:%.*]]
+; CHECK-NEXT:    [[BRMERGE:%.*]] = select i1 [[A_NOT]], i1 true, i1 [[B:%.*]], !prof [[PROF0:![0-9]+]]
+; CHECK-NEXT:    br i1 [[BRMERGE]], label [[B:%.*]], label [[COMMON_RET:%.*]], !prof [[PROF0]]
 ; CHECK:       common.ret:
 ; CHECK-NEXT:    ret void
 ; CHECK:       b:
@@ -15,9 +15,9 @@ define void @test(ptr %P, ptr %Q, i1 %A, i1 %B) {
 ;
 
 entry:
-  br i1 %A, label %a, label %b
+  br i1 %A, label %a, label %b, !prof !0
 a:
-  br i1 %B, label %b, label %c
+  br i1 %B, label %b, label %c, !prof !1
 b:
   store i32 123, ptr %P
   ret void
@@ -146,3 +146,11 @@ Succ:
 }
 
 declare void @dummy()
+
+!0 = !{!"branch_weights", i32 3, i32 7}
+!1 = !{!"branch_weights", i32 11, i32 4}
+;.
+; CHECK: attributes #[[ATTR0:[0-9]+]] = { nounwind ssp memory(read) uwtable }
+;.
+; CHECK: [[PROF0]] = !{!"branch_weights", i32 138, i32 12}
+;.
diff --git a/llvm/test/Transforms/SimplifyCFG/preserve-branchweights.ll b/llvm/test/Transforms/SimplifyCFG/preserve-branchweights.ll
index 0f78e236b4248..7d1153d5c0a0e 100644
--- a/llvm/test/Transforms/SimplifyCFG/preserve-branchweights.ll
+++ b/llvm/test/Transforms/SimplifyCFG/preserve-branchweights.ll
@@ -268,8 +268,8 @@ define void @test7(i1 %a, i1 %b) {
 ; CHECK-LABEL: @test7(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[C:%.*]] = or i1 [[B:%.*]], false
-; CHECK-NEXT:    [[BRMERGE:%.*]] = select i1 [[A:%.*]], i1 true, i1 [[C]]
-; CHECK-NEXT:    br i1 [[BRMERGE]], label [[Y:%.*]], label [[Z:%.*]], !prof [[PROF6:![0-9]+]]
+; CHECK-NEXT:    [[BRMERGE:%.*]] = select i1 [[A:%.*]], i1 true, i1 [[C]], !prof [[PROF6:![0-9]+]]
+; CHECK-NEXT:    br i1 [[BRMERGE]], label [[Y:%.*]], label [[Z:%.*]], !prof [[PROF6]]
 ; CHECK:       common.ret:
 ; CHECK-NEXT:    ret void
 ; CHECK:       Y:
@@ -557,9 +557,9 @@ return:
 define i32 @SimplifyCondBranchToCondBranch(i1 %cmpa, i1 %cmpb) {
 ; CHECK-LABEL: @SimplifyCondBranchToCondBranch(
 ; CHECK-NEXT:  block1:
-; CHECK-NEXT:    [[BRMERGE:%.*]] = select i1 [[CMPA:%.*]], i1 true, i1 [[CMPB:%.*]]
-; CHECK-NEXT:    [[DOTMUX:%.*]] = select i1 [[CMPA]], i32 0, i32 2, !prof [[PROF13:![0-9]+]]
-; CHECK-NEXT:    [[OUTVAL:%.*]] = select i1 [[BRMERGE]], i32 [[DOTMUX]], i32 1, !prof [[PROF14:![0-9]+]]
+; CHECK-NEXT:    [[BRMERGE:%.*]] = select i1 [[CMPA:%.*]], i1 true, i1 [[CMPB:%.*]], !prof [[PROF13:![0-9]+]]
+; CHECK-NEXT:    [[DOTMUX:%.*]] = select i1 [[CMPA]], i32 0, i32 2, !prof [[PROF14:![0-9]+]]
+; CHECK-NEXT:    [[OUTVAL:%.*]] = select i1 [[BRMERGE]], i32 [[DOTMUX]], i32 1, !prof [[PROF13]]
 ; CHECK-NEXT:    ret i32 [[OUTVAL]]
 ;
 block1:
@@ -584,9 +584,9 @@ define i32 @SimplifyCondBranchToCondBranchSwap(i1 %cmpa, i1 %cmpb) {
 ; CHECK-NEXT:  block1:
 ; CHECK-NEXT:    [[CMPA_NOT:%.*]] = xor i1 [[CMPA:%.*]], true
 ; CHECK-NEXT:    [[CMPB_NOT:%.*]] = xor i1 [[CMPB:%.*]], true
-; CHECK-NEXT:    [[BRMERGE:%.*]] = select i1 [[CMPA_NOT]], i1 true, i1 [[CMPB_NOT]]
-; CHECK-NEXT:    [[DOTMUX:%.*]] = select i1 [[CMPA_NOT]], i32 0, i32 2, !prof [[PROF15:![0-9]+]]
-; CHECK-NEXT:    [[OUTVAL:%.*]] = select i1 [[BRMERGE]], i32 [[DOTMUX]], i32 1, !prof [[PROF16:![0-9]+]]
+; CHECK-NEXT:    [[BRMERGE:%.*]] = select i1 [[CMPA_NOT]], i1 true, i1 [[CMPB_NOT]], !prof [[PROF15:![0-9]+]]
+; CHECK-NEXT:    [[DOTMUX:%.*]] = select i1 [[CMPA_NOT]], i32 0, i32 2, !prof [[PROF16:![0-9]+]]
+; CHECK-NEXT:    [[OUTVAL:%.*]] = select i1 [[BRMERGE]], i32 [[DOTMUX]], i32 1, !prof [[PROF15]]
 ; CHECK-NEXT:    ret i32 [[OUTVAL]]
 ;
 block1:
@@ -609,9 +609,9 @@ define i32 @SimplifyCondBranchToCondBranchSwapMissingWeight(i1 %cmpa, i1 %cmpb)
 ; CHECK-NEXT:  block1:
 ; CHECK-NEXT:    [[CMPA_NOT:%.*]] = xor i1 [[CMPA:%.*]], true
 ; CHECK-NEXT:    [[CMPB_NOT:%.*]] = xor i1 [[CMPB:%.*]], true
-; CHECK-NEXT:    [[BRMERGE:%.*]] = select i1 [[CMPA_NOT]], i1 true, i1 [[CMPB_NOT]]
-; CHECK-NEXT:    [[DOTMUX:%.*]] = select i1 [[CMPA_NOT]], i32 0, i32 2, !prof [[PROF17:![0-9]+]]
-; CHECK-NEXT:    [[OUTVAL:%.*]] = select i1 [[BRMERGE]], i32 [[DOTMUX]], i32 1, !prof [[PROF18:![0-9]+]]
+; CHECK-NEXT:    [[BRMERGE:%.*]] = select i1 [[CMPA_NOT]], i1 true, i1 [[CMPB_NOT]], !prof [[PROF17:![0-9]+]]
+; CHECK-NEXT:    [[DOTMUX:%.*]] = select i1 [[CMPA_NOT]], i32 0, i32 2, !prof [[PROF18:![0-9]+]]
+; CHECK-NEXT:    [[OUTVAL:%.*]] = select i1 [[BRMERGE]], i32 [[DOTMUX]], i32 1, !prof [[PROF17]]
 ; CHECK-NEXT:    ret i32 [[OUTVAL]]
 ;
 block1:
@@ -1114,12 +1114,12 @@ exit:
 ; CHECK: [[PROF10]] = !{!"branch_weights", i32 8, i32 33}
 ; CHECK: [[PROF11]] = !{!"branch_weights", i32 112017436, i32 -735157296}
 ; CHECK: [[PROF12]] = !{!"branch_weights", i32 3, i32 5}
-; CHECK: [[PROF13]] = !{!"branch_weights", i32 22, i32 12}
-; CHECK: [[PROF14]] = !{!"branch_weights", i32 34, i32 21}
-; CHECK: [[PROF15]] = !{!"branch_weights", i32 33, i32 14}
-; CHECK: [[PROF16]] = !{!"branch_weights", i32 47, i32 8}
-; CHECK: [[PROF17]] = !{!"branch_weights", i32 6, i32 2}
-; CHECK: [[PROF18]] = !{!"branch_weights", i32 8, i32 2}
+; CHECK: [[PROF13]] = !{!"branch_weights", i32 34, i32 21}
+; CHECK: [[PROF14]] = !{!"branch_weights", i32 22, i32 12}
+; CHECK: [[PROF15]] = !{!"branch_weights", i32 47, i32 8}
+; CHECK: [[PROF16]] = !{!"branch_weights", i32 33, i32 14}
+; CHECK: [[PROF17]] = !{!"branch_weights", i32 8, i32 2}
+; CHECK: [[PROF18]] = !{!"branch_weights", i32 6, i32 2}
 ; CHECK: [[PROF19]] = !{!"branch_weights", i32 99, i32 1}
 ; CHECK: [[PROF20]] = !{!"branch_weights", i32 1, i32 99}
 ; CHECK: [[PROF21]] = !{!"branch_weights", i32 199, i32 1}

@mtrofin mtrofin force-pushed the users/mtrofin/08-19-_simplfycfg_set_md_prof_for_select_used_for_certain_conditional_simplifications branch from af93acd to 37e4c28 Compare August 20, 2025 00:23
@mtrofin mtrofin requested a review from nikic August 20, 2025 00:24
if (!ProfcheckDisableMetadataFixes)
if (auto *SI = dyn_cast<SelectInst>(PBI->getCondition()))
if (!MDWeights.empty()) {
assert(isSelectInRoleOfConjunctionOrDisjunction(SI));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I preferred assert-ing here (and below) because the expectation is the condition comes from createLogicalOp and the only reason we get a select is because of how or or and get lowered there.

@mtrofin mtrofin requested review from alanzhao1 and rnk August 21, 2025 00:34
@mtrofin mtrofin force-pushed the users/mtrofin/08-19-_simplfycfg_set_md_prof_for_select_used_for_certain_conditional_simplifications branch 2 times, most recently from 961489a to 386761d Compare August 25, 2025 20:17
@mtrofin mtrofin requested a review from snehasish August 25, 2025 20:19
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure on this one. The thing is that the canonical form here is a plain and/or, and the only reason we use select is to prevent poison-propagation issues. Improvements to frontend information (e.g. if you slap noundef on the relevant arguments) or analysis may end up converting the select to and/or, in which case we won't be able to preserve the prof metadata. Cases where better analysis has adverse effects are always awkward.

Do you have an example where duplicating this information on the select ends up being useful? After all, these weights are already on the branch over the select.

@mtrofin
Copy link
Member Author

mtrofin commented Aug 26, 2025

I'm not sure on this one. The thing is that the canonical form here is a plain and/or, and the only reason we use select is to prevent poison-propagation issues. Improvements to frontend information (e.g. if you slap noundef on the relevant arguments) or analysis may end up converting the select to and/or, in which case we won't be able to preserve the prof metadata. Cases where better analysis has adverse effects are always awkward.
Do you have an example where duplicating this information on the select ends up being useful? After all, these weights are already on the branch over the select.

The approach is defensive rather than example-driven, because at large scale, places where profile information starts deteriorating are hard to identify, and it's also hard to know there's a problem - hence the approach of "if an instruction can have MD_profile, and it's known, let's capture it, otherwise mark it as unknown". This is compounded by changes that break downstream assumptions. In this case, the profile info is easily known, so it seems prudent to capture it rather than marking it unknown.

Not sure what you mean by "better analysis has adverse effects"?

@mtrofin mtrofin force-pushed the users/mtrofin/08-19-_simplfycfg_set_md_prof_for_select_used_for_certain_conditional_simplifications branch from 386761d to 4fea73e Compare August 27, 2025 17:13
@mtrofin mtrofin changed the base branch from main to users/mtrofin/08-28-_simplifycfg_probabilities_associated_with_same_condition_are_constant August 28, 2025 01:36
@mtrofin mtrofin force-pushed the users/mtrofin/08-19-_simplfycfg_set_md_prof_for_select_used_for_certain_conditional_simplifications branch from 4fea73e to 34f5a42 Compare August 28, 2025 01:36
@mtrofin mtrofin force-pushed the users/mtrofin/08-19-_simplfycfg_set_md_prof_for_select_used_for_certain_conditional_simplifications branch from 34f5a42 to 5427cb6 Compare September 4, 2025 17:28
@mtrofin mtrofin force-pushed the users/mtrofin/08-28-_simplifycfg_probabilities_associated_with_same_condition_are_constant branch from c61eb49 to 9d2d78e Compare September 4, 2025 17:28
fitWeights(NewWeights);

SmallVector<uint32_t, 8> MDWeights(NewWeights.begin(), NewWeights.end());
append_range(MDWeights, NewWeights);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need MDWeights in the newly introduced code.

@mtrofin mtrofin force-pushed the users/mtrofin/08-19-_simplfycfg_set_md_prof_for_select_used_for_certain_conditional_simplifications branch from 5427cb6 to 1f876ac Compare September 4, 2025 17:53
@mtrofin mtrofin force-pushed the users/mtrofin/08-28-_simplifycfg_probabilities_associated_with_same_condition_are_constant branch from 9d2d78e to 7b84436 Compare September 4, 2025 21:02
@mtrofin mtrofin force-pushed the users/mtrofin/08-19-_simplfycfg_set_md_prof_for_select_used_for_certain_conditional_simplifications branch from 1f876ac to 80e4431 Compare September 4, 2025 21:02
Base automatically changed from users/mtrofin/08-28-_simplifycfg_probabilities_associated_with_same_condition_are_constant to main September 4, 2025 21:37
@nikic
Copy link
Contributor

nikic commented Sep 4, 2025

I'm not sure on this one. The thing is that the canonical form here is a plain and/or, and the only reason we use select is to prevent poison-propagation issues. Improvements to frontend information (e.g. if you slap noundef on the relevant arguments) or analysis may end up converting the select to and/or, in which case we won't be able to preserve the prof metadata. Cases where better analysis has adverse effects are always awkward.
Do you have an example where duplicating this information on the select ends up being useful? After all, these weights are already on the branch over the select.

The approach is defensive rather than example-driven, because at large scale, places where profile information starts deteriorating are hard to identify, and it's also hard to know there's a problem - hence the approach of "if an instruction can have MD_profile, and it's known, let's capture it, otherwise mark it as unknown". This is compounded by changes that break downstream assumptions. In this case, the profile info is easily known, so it seems prudent to capture it rather than marking it unknown.

Not sure what you mean by "better analysis has adverse effects"?

If you add noundef to the parameters for the condition, then we'll generate and/or instead of select, which can't carry !prof metadata.

Copy link
Member Author

mtrofin commented Sep 4, 2025

That's OK, the !prof stuff is only added if the instruction is a SelectInst (unless I missed something, at least that's what I think I wrote 😄 )

@mtrofin mtrofin force-pushed the users/mtrofin/08-19-_simplfycfg_set_md_prof_for_select_used_for_certain_conditional_simplifications branch 2 times, most recently from a9fc269 to a8bb3f0 Compare September 5, 2025 23:34
Copy link
Member Author

mtrofin commented Sep 8, 2025

@nikic am I OK to push? Thanks!

@mtrofin mtrofin force-pushed the users/mtrofin/08-19-_simplfycfg_set_md_prof_for_select_used_for_certain_conditional_simplifications branch from a8bb3f0 to a80b9ee Compare September 11, 2025 20:29
Copy link
Member Author

mtrofin commented Sep 12, 2025

Merge activity

  • Sep 12, 2:49 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Sep 12, 2:50 PM UTC: @mtrofin merged this pull request with Graphite.

@mtrofin mtrofin merged commit 889c289 into main Sep 12, 2025
9 checks passed
@mtrofin mtrofin deleted the users/mtrofin/08-19-_simplfycfg_set_md_prof_for_select_used_for_certain_conditional_simplifications branch September 12, 2025 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants