[StructurizeCFG] bug fix in zero cost hoist (#157969)

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.

Author: VigneshwarJ

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;
 
@@ -432,27 +445,18 @@ static bool isHoistableInstruction(Instruction *I, BasicBlock *BB,
   if (CostVal != 0)
     return false;
 
-  // Check if any operands are instructions defined in the same block.
+  // Check if all operands are available at the hoisting destination.
   for (auto &Op : I->operands()) {
     if (auto *OpI = dyn_cast<Instruction>(Op)) {
-      if (OpI->getParent() == BB)
+      // Operand must dominate the hoisting destination.
+      if (!DT->dominates(OpI->getParent(), HoistTo))
         return false;
     }
   }
 
   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 +482,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());

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
+}