[StructurizeCFG] bug fix in zero cost hoist #157969
Merged
Conversation
This file contains hidden or 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
This fixes a bug where zero cost instruction was hoisted to nearest common dominator but the hoisted instruction's operands didn't dominate the common dominator causing poison values.
|
@llvm/pr-subscribers-llvm-transforms Author: Vigneshwar Jayakumar (VigneshwarJ) ChangesThis fixes a bug where zero cost instruction was hoisted to nearest common dominator but the hoisted instruction's operands didn't dominate the common dominator causing poison values. Full diff: https://github.com/llvm/llvm-project/pull/157969.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp b/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
index e05625344ee29..72f96e1a27f0b 100644
--- a/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
+++ b/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
@@ -308,6 +308,9 @@ class StructurizeCFG {
void hoistZeroCostElseBlockPhiValues(BasicBlock *ElseBB, BasicBlock *ThenBB);
+ bool isHoistableInstruction(Instruction *I, BasicBlock *BB,
+ BasicBlock *HoistTo);
+
void orderNodes();
void analyzeLoops(RegionNode *N);
@@ -415,11 +418,21 @@ class StructurizeCFGLegacyPass : public RegionPass {
} // end anonymous namespace
+char StructurizeCFGLegacyPass::ID = 0;
+
+INITIALIZE_PASS_BEGIN(StructurizeCFGLegacyPass, "structurizecfg",
+ "Structurize the CFG", false, false)
+INITIALIZE_PASS_DEPENDENCY(UniformityInfoWrapperPass)
+INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
+INITIALIZE_PASS_DEPENDENCY(RegionInfoPass)
+INITIALIZE_PASS_END(StructurizeCFGLegacyPass, "structurizecfg",
+ "Structurize the CFG", false, false)
+
/// Checks whether an instruction is zero cost instruction and checks if the
/// operands are from different BB. If so, this instruction can be coalesced
/// if its hoisted to predecessor block. So, this returns true.
-static bool isHoistableInstruction(Instruction *I, BasicBlock *BB,
- const TargetTransformInfo *TTI) {
+bool StructurizeCFG::isHoistableInstruction(Instruction *I, BasicBlock *BB,
+ BasicBlock *HoistTo) {
if (I->getParent() != BB || isa<PHINode>(I))
return false;
@@ -435,7 +448,7 @@ static bool isHoistableInstruction(Instruction *I, BasicBlock *BB,
// Check if any operands are instructions defined in the same block.
for (auto &Op : I->operands()) {
if (auto *OpI = dyn_cast<Instruction>(Op)) {
- if (OpI->getParent() == BB)
+ if (OpI->getParent() == BB || !DT->dominates(OpI->getParent(), HoistTo))
return false;
}
}
@@ -443,16 +456,6 @@ static bool isHoistableInstruction(Instruction *I, BasicBlock *BB,
return true;
}
-char StructurizeCFGLegacyPass::ID = 0;
-
-INITIALIZE_PASS_BEGIN(StructurizeCFGLegacyPass, "structurizecfg",
- "Structurize the CFG", false, false)
-INITIALIZE_PASS_DEPENDENCY(UniformityInfoWrapperPass)
-INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
-INITIALIZE_PASS_DEPENDENCY(RegionInfoPass)
-INITIALIZE_PASS_END(StructurizeCFGLegacyPass, "structurizecfg",
- "Structurize the CFG", false, false)
-
/// Structurization can introduce unnecessary VGPR copies due to register
/// coalescing interference. For example, if the Else block has a zero-cost
/// instruction and the Then block modifies the VGPR value, only one value is
@@ -478,7 +481,7 @@ void StructurizeCFG::hoistZeroCostElseBlockPhiValues(BasicBlock *ElseBB,
for (PHINode &Phi : ElseSucc->phis()) {
Value *ElseVal = Phi.getIncomingValueForBlock(ElseBB);
auto *Inst = dyn_cast<Instruction>(ElseVal);
- if (!Inst || !isHoistableInstruction(Inst, ElseBB, TTI))
+ if (!Inst || !isHoistableInstruction(Inst, ElseBB, CommonDominator))
continue;
Inst->removeFromParent();
Inst->insertInto(CommonDominator, Term->getIterator());
diff --git a/llvm/test/Transforms/StructurizeCFG/hoist-zerocost.ll b/llvm/test/Transforms/StructurizeCFG/hoist-zerocost.ll
index d084e199ceb89..b118f8189d716 100644
--- a/llvm/test/Transforms/StructurizeCFG/hoist-zerocost.ll
+++ b/llvm/test/Transforms/StructurizeCFG/hoist-zerocost.ll
@@ -209,3 +209,53 @@ merge:
store i32 %phi, ptr %ptr
ret void
}
+
+define void @test_nested_if_2 (i32 %val,ptr %gep, i1 %cond) {
+; CHECK-LABEL: define void @test_nested_if_2(
+; CHECK-SAME: i32 [[VAL:%.*]], ptr [[GEP:%.*]], i1 [[COND:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: [[COND_INV:%.*]] = xor i1 [[COND]], true
+; CHECK-NEXT: [[T825:%.*]] = icmp eq i32 [[VAL]], 0
+; CHECK-NEXT: [[T825_INV:%.*]] = xor i1 [[T825]], true
+; CHECK-NEXT: br i1 [[T825]], label %[[IF:.*]], label %[[FLOW:.*]]
+; CHECK: [[IF]]:
+; CHECK-NEXT: [[LOADED:%.*]] = load [[PAIR:%.*]], ptr [[GEP]], align 4
+; CHECK-NEXT: br label %[[FLOW]]
+; CHECK: [[FLOW1:.*]]:
+; CHECK-NEXT: [[TMP0:%.*]] = phi i1 [ false, %[[ELSE:.*]] ], [ [[TMP2:%.*]], %[[FLOW]] ]
+; CHECK-NEXT: br i1 [[TMP0]], label %[[IF_2:.*]], label %[[EXIT:.*]]
+; CHECK: [[IF_2]]:
+; CHECK-NEXT: [[IF_VALUE:%.*]] = extractvalue [[PAIR]] [[TMP1:%.*]], 0
+; CHECK-NEXT: br label %[[EXIT]]
+; CHECK: [[FLOW]]:
+; CHECK-NEXT: [[TMP1]] = phi [[PAIR]] [ [[LOADED]], %[[IF]] ], [ poison, %[[ENTRY]] ]
+; CHECK-NEXT: [[TMP2]] = phi i1 [ true, %[[IF]] ], [ false, %[[ENTRY]] ]
+; CHECK-NEXT: [[TMP3:%.*]] = phi i1 [ [[COND_INV]], %[[IF]] ], [ [[T825_INV]], %[[ENTRY]] ]
+; CHECK-NEXT: br i1 [[TMP3]], label %[[ELSE]], label %[[FLOW1]]
+; CHECK: [[ELSE]]:
+; CHECK-NEXT: br label %[[FLOW1]]
+; CHECK: [[EXIT]]:
+; CHECK-NEXT: [[T_SINK168:%.*]] = phi i32 [ 0, %[[FLOW1]] ], [ [[IF_VALUE]], %[[IF_2]] ]
+; CHECK-NEXT: store i32 [[T_SINK168]], ptr [[GEP]], align 4
+; CHECK-NEXT: ret void
+;
+entry:
+ %t825 = icmp eq i32 %val, 0
+ br i1 %t825, label %if, label %else
+
+if:
+ %loaded = load %pair, ptr %gep
+ br i1 %cond, label %if_2, label %else
+
+if_2:
+ %if_value = extractvalue %pair %loaded, 0
+ br label %exit
+
+else:
+ br label %exit
+
+exit:
+ %phi = phi i32 [ %if_value, %if_2 ], [ 0, %else ]
+ store i32 %phi,ptr %gep
+ ret void
+}
|
dstutt
approved these changes
Sep 11, 2025
jayfoad
reviewed
Sep 11, 2025
| if (CostVal != 0) | ||
| return false; | ||
|
|
||
| // Check if any operands are instructions defined in the same block. |
| for (auto &Op : I->operands()) { | ||
| if (auto *OpI = dyn_cast<Instruction>(Op)) { | ||
| if (OpI->getParent() == BB) | ||
| if (OpI->getParent() == BB || !DT->dominates(OpI->getParent(), HoistTo)) |
There was a problem hiding this comment.
Do you still need the == BB check? Shouldn't it be subsumed by the dominates check?
There was a problem hiding this comment.
oh yes, updated.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/31148 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/25541 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
5 participants
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.
This fixes a bug where zero cost instruction was hoisted to nearest common dominator but the hoisted instruction's operands didn't dominate the common dominator causing poison values.