[SimplifyCFG] Simplify conditional branches on const icmp eq's

[yonillasky]

The issue that is being addressed here is shown in the @ intersection_block_with_dead_predecessor test.

Before this fix, what happens there is that:

• SimplifyCFG converts the switch instruction into a conditional branch (on the PHI value)
• SimplifyCFG identifies the block %a is dead, and deletes it
• PHI decays into a constant
• The condition of the conditional branch is now a compile-time constant, so one of the outgoing edges is dead, but no further simplification is performed (though here, for instance, %c is a dead block)

I am fixing that, by explicitly doing the necessary constant-folding for this specific case.

[llvmbot]

@llvm/pr-subscribers-coroutines
@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: None (yonillasky)

Changes

The issue that is being addressed here is shown in the @ intersection_block_with_dead_predecessor test.

Before this fix, what happens there is that:

• SimplifyCFG converts the switch instruction into a conditional branch (on the PHI value)
• SimplifyCFG identifies the block %a is dead, and deletes it
• PHI decays into a constant
• The condition of the conditional branch is now a compile-time constant, so one of the outgoing edges is dead, but no further simplification is performed (though here, for instance, %c is a dead block)

I am fixing that, by explicitly doing the necessary constant-folding for this specific case.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/SimplifyCFG.cpp (+18)
• (added) llvm/test/Transforms/SimplifyCFG/constant-valued-cond-br.ll (+47)

diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 3bcd896639a8ec2..2fe0c281662aa36 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -3578,6 +3578,13 @@ static bool FoldTwoEntryPHINode(PHINode *PN, const TargetTransformInfo &TTI,
   return true;
 }
 
+static BranchInst *decayCondBranchToUncondBranch(IRBuilderBase &Builder, BranchInst *BI, bool Eval) {
+  unsigned SuccessorIdx = (Eval) ? 0 : 1;
+  auto *NewBI = Builder.CreateBr(BI->getSuccessor(SuccessorIdx));
+  BI->eraseFromParent();
+  return NewBI;
+}
+
 static Value *createLogicalOp(IRBuilderBase &Builder,
                               Instruction::BinaryOps Opc, Value *LHS,
                               Value *RHS, const Twine &Name = "") {
@@ -7325,6 +7332,17 @@ bool SimplifyCFGOpt::simplifyCondBranch(BranchInst *BI, IRBuilder<> &Builder) {
           if (mergeConditionalStores(PBI, BI, DTU, DL, TTI))
             return requestResimplify();
 
+  // Check if the condition is an equality between two constants. This can form due to other
+  // CFGSimplify steps, and may prevent further simplification if we don't deal with it here.
+  if (auto ICmp = dyn_cast<ICmpInst>(BI->getCondition()))
+    if (ICmp->getPredicate() == CmpInst::ICMP_EQ)
+      if (auto *LHS = dyn_cast<ConstantInt>(ICmp->getOperand(0)))
+        if (auto *RHS = dyn_cast<ConstantInt>(ICmp->getOperand(1))) {
+          bool CondEval = LHS->getZExtValue() == RHS->getZExtValue();
+          decayCondBranchToUncondBranch(Builder, BI, CondEval);
+          return requestResimplify();
+        }
+
   return false;
 }
 
diff --git a/llvm/test/Transforms/SimplifyCFG/constant-valued-cond-br.ll b/llvm/test/Transforms/SimplifyCFG/constant-valued-cond-br.ll
new file mode 100644
index 000000000000000..51262dab3ab0436
--- /dev/null
+++ b/llvm/test/Transforms/SimplifyCFG/constant-valued-cond-br.ll
@@ -0,0 +1,47 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt < %s -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S | FileCheck %s
+
+define void @const_valued_cond_br(ptr %P) {
+; CHECK-LABEL: define void @const_valued_cond_br(
+; CHECK-SAME: ptr [[P:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[COND:%.*]] = icmp eq i32 42, 42
+; CHECK-NEXT:    store i32 123, ptr [[P]], align 4
+; CHECK-NEXT:    ret void
+;
+entry:
+  %cond = icmp eq i32 42, 42
+  br i1 %cond, label %a, label %b
+a:
+  store i32 123, ptr %P
+  br label %b
+b:
+  ret void
+}
+
+
+
+define void @intersection_block_with_dead_predecessor(ptr %P) {
+; CHECK-LABEL: define void @intersection_block_with_dead_predecessor(
+; CHECK-SAME: ptr [[P:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[COND:%.*]] = icmp eq i32 1, 1
+; CHECK-NEXT:    store i32 321, ptr [[P]], align 4
+; CHECK-NEXT:    ret void
+;
+entry:
+  br label %b
+b:
+  %x = phi i32 [1, %entry], [2, %a]
+  switch i32 %x, label %c [
+  i32 1, label %d
+  ]
+c:
+  store i32 123, ptr %P
+  ret void
+d:
+  store i32 321, ptr %P
+  ret void
+a: ; unreachable
+  br label %b
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[nikic]

What is the broader motivation for this? That is, why is it important to handle this within a single SimplifyCFG invocation? We'd probably want to have a PhaseOrdering test for that.

I'm not entirely sure whether this is the best approach. A possible alternative would be to instead perform additional simplification when removing the unreachable block and simplifying the phi to a constant. We could try to simplify all instructions that used the phi at that point.

[yonillasky]

What is the broader motivation for this? That is, why is it important to handle this within a single SimplifyCFG invocation? We'd probably want to have a PhaseOrdering test for that.

Pay close attention to the test. To fully simplify the test case with existing code you'd need to run SimplifyCFG first, then a pass that folds constants, then SimplifyCFG again.
That, to me, seemed unreasonable - hence the PR.

I'm not entirely sure whether this is the best approach. A possible alternative would be to instead perform additional simplification when removing the unreachable block and simplifying the phi to a constant. We could try to simplify all instructions that used the phi at that point.

I like the idea. However, how about specifically attempting to constant-fold the successor's BranchInst condition, if it has one, then?
I'm making this counter-offer since I'm not sure the CFG simplification pass should be also doing constant-folding in unrelated instructions.

[yonillasky]

So ... the suggested alternative is to replace all calls to DeleteDeadBlock with a new function that has the same semantics, but also lets us act upon 1-input PHIs being decayed into constants?

[nikic]

What is the broader motivation for this? That is, why is it important to handle this within a single SimplifyCFG invocation? We'd probably want to have a PhaseOrdering test for that.

Pay close attention to the test. To fully simplify the test case with existing code you'd need to run SimplifyCFG first, then a pass that folds constants, then SimplifyCFG again. That, to me, seemed unreasonable - hence the PR.

It is generally necessary to run SimplifyCFG, InstCombine and SimplifyCFG again to get most optimization opportunities. It can make sense to short-cut this in specific cases for phase ordering reasons, but we should have specific motivation for that. That's why I'm asking what your original motivation here is.

Another possibility is to make InstCombine slightly stronger so it can fold the phi away without a prior SimplifyCFG run -- in fact, it already does so for blocks that are dynamically unreachable, but fails to do that for blocks that are statically unreachable. That difference in not intended and just an implementation artifact.

[yonillasky]

The scenario that prompted me to make the change was that in CoroCleanup it explicitly makes a SimplifyCFG call on all affected coroutines, I saw it fail to fully optimize the CFG in a certain scenario.

Should it expect that InstCombine / SimplifyCFG will run afterwards and fix whatever problems remain? I thought the simplify being there meant that the coro pipeline was expected to output "cleaned up" code already -- there are very few passes after it... but honestly I'm not that familiar with all the various pipelines and pass schedules that LLVM has.

[yonillasky]

I think I can make the change you suggested without too much trouble. I'll do it and update the PR shortly.

[yonillasky]

I've sort of managed to implement the CR suggestion.
I'm intercepting only the BB removals inside simplifyFunctionCFGImpl, not the 5 use cases of DeleteDeadBlock inside the SimplifyCFG modules. It seems to be sufficient for the use case I am interested in.
Will run and fix UTs as necessary. It should have a smaller effect now, since it only fixes the problem if SimplifyCFG was the one to introduce it.