[InstCombine] Preload DomConditionCache to reach fixed point when sinking

[gargaroff]

Preload the DomConditionCache when preparing the worklist to ensure that
branches have been seen before sunk instructions are visited again.
This fixes test2 in sink_instruction.ll which was not reaching a fixed
point previously because the sunk instructions were revisited too early
before the branch condition was visited.

Fixes #77462

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Dominik Montada (gargaroff)

Changes

Preload the DomConditionCache when preparing the worklist to ensure that
branches have been seen before sunk instructions are visited again.
This fixes test2 in sink_instruction.ll which was not reaching a fixed
point previously because the sunk instructions were revisited too early
before the branch condition was visited.

Fixes #77462

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+1)
• (modified) llvm/test/Transforms/InstCombine/sink_instruction.ll (+8-12)

diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index c6de57cb34c69..74c2d6d8a1a6e 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -5902,6 +5902,7 @@ bool InstCombinerImpl::prepareWorklist(Function &F) {
     // live successor. Otherwise assume all successors are live.
     Instruction *TI = BB->getTerminator();
     if (BranchInst *BI = dyn_cast<BranchInst>(TI); BI && BI->isConditional()) {
+      DC.registerBranch(BI);
       if (isa<UndefValue>(BI->getCondition())) {
         // Branch on undef is UB.
         HandleOnlyLiveSuccessor(BB, nullptr);
diff --git a/llvm/test/Transforms/InstCombine/sink_instruction.ll b/llvm/test/Transforms/InstCombine/sink_instruction.ll
index cb9a3069ca5fd..ea8b572f32541 100644
--- a/llvm/test/Transforms/InstCombine/sink_instruction.ll
+++ b/llvm/test/Transforms/InstCombine/sink_instruction.ll
@@ -27,32 +27,28 @@ endif:          ; preds = %entry
   ret i32 %tmp.2
 }
 
-; We fail to reach a fixpoint, because sunk instructions get revisited too
-; early. In @test2 the sunk add is revisited before the dominating condition
-; is visited and added to the DomConditionCache.
+; This used to fail to reach a fixpoint, because sunk instructions got
+; revisited before the dominating condition was visited and added to the
+; DomConditionCache.
 
 ;; PHI use, sink divide before call.
-define i32 @test2(i32 %x) nounwind ssp "instcombine-no-verify-fixpoint" {
+define i32 @test2(i32 %x) nounwind ssp {
 ; CHECK-LABEL: @test2(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br label [[BB:%.*]]
 ; CHECK:       bb:
-; CHECK-NEXT:    [[X_ADDR_17:%.*]] = phi i32 [ [[X:%.*]], [[ENTRY:%.*]] ], [ [[X_ADDR_0:%.*]], [[BB2:%.*]] ]
-; CHECK-NEXT:    [[I_06:%.*]] = phi i32 [ 0, [[ENTRY]] ], [ [[TMP4:%.*]], [[BB2]] ]
-; CHECK-NEXT:    [[TMP0:%.*]] = icmp eq i32 [[X_ADDR_17]], 0
+; CHECK-NEXT:    [[I_06:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[TMP4:%.*]], [[BB2:%.*]] ]
+; CHECK-NEXT:    [[TMP0:%.*]] = icmp eq i32 [[X_ADDR_17:%.*]], 0
 ; CHECK-NEXT:    br i1 [[TMP0]], label [[BB1:%.*]], label [[BB2]]
 ; CHECK:       bb1:
-; CHECK-NEXT:    [[TMP1:%.*]] = add nsw i32 [[X_ADDR_17]], 1
-; CHECK-NEXT:    [[TMP2:%.*]] = sdiv i32 [[TMP1]], [[X_ADDR_17]]
-; CHECK-NEXT:    [[TMP3:%.*]] = tail call i32 @bar() #[[ATTR3:[0-9]+]]
+; CHECK-NEXT:    [[TMP1:%.*]] = tail call i32 @bar() #[[ATTR3:[0-9]+]]
 ; CHECK-NEXT:    br label [[BB2]]
 ; CHECK:       bb2:
-; CHECK-NEXT:    [[X_ADDR_0]] = phi i32 [ [[TMP2]], [[BB1]] ], [ [[X_ADDR_17]], [[BB]] ]
 ; CHECK-NEXT:    [[TMP4]] = add nuw nsw i32 [[I_06]], 1
 ; CHECK-NEXT:    [[EXITCOND:%.*]] = icmp eq i32 [[TMP4]], 1000000
 ; CHECK-NEXT:    br i1 [[EXITCOND]], label [[BB4:%.*]], label [[BB]]
 ; CHECK:       bb4:
-; CHECK-NEXT:    ret i32 [[X_ADDR_0]]
+; CHECK-NEXT:    ret i32 [[X_ADDR_17]]
 ;
 entry:
   br label %bb

[nikic]

Looks like doing this has a large compile-time overhead: https://llvm-compile-time-tracker.com/compare.php?from=d5551e1d595a5298f11f2cf1317799a2680bd7ce&to=3752f2bbbc13fb622b68fc02371a9c086b9ad8b2&stat=instructions:u

[gargaroff]

That's a shame. Would it be acceptable to only do this when sinking triggers?
We're hitting this bug in our downstream fork on real life content so our only option is to either increase the InstCombine iteration limit to 2 or to fix this bug.

[gargaroff]

I guess another option could be to add conditional branches of all predecessors of the sink block to the worklist when sinking, not just the sunk operation.

[nikic]

We're hitting this bug in our downstream fork on real life content so our only option is to either increase the InstCombine iteration limit to 2 or to fix this bug.

Then you are invoking InstCombine incorrectly. Production use must use no-verify-fixpoint, which is the default when creating an InstCombine instance via the C++ API. If you are creating it via a pass pipeline string, you need to specify this explicitly. verify-fixpoint is a testing facility and expected to fail.

[gargaroff]

We're hitting this bug in our downstream fork on real life content so our only option is to either increase the InstCombine iteration limit to 2 or to fix this bug.

Then you are invoking InstCombine incorrectly. Production use must use no-verify-fixpoint, which is the default when creating an InstCombine instance via the C++ API. If you are creating it via a pass pipeline string, you need to specify this explicitly. verify-fixpoint is a testing facility and expected to fail.

Oh that's good to know, had no idea!
I'm gonna double check that in our fork, but would still be nice if we could fix this issue. I'm gonna check whether I can come up with a solution that is less impactful on compile time.