[ConstraintElim] Extend checkOrAndOpImpliedByOther to handle and/or expr trees.

[dtcxzyw]

This patch extends checkOrAndOpImpliedByOther to handle and/or trees.
Limitation: At least one of the operands of root and/or instruction should be an icmp. That is, this patch doesn't support expressions like (cmp1 & cmp2) & (cmp3 & cmp4). To handle this kind of pattern, please see #117118.

Closes #117107.
Compile-time impact: http://llvm-compile-time-tracker.com/compare.php?from=69cc3f096ccbdef526bbd5a065a25c95122e87ee&to=919416d2c4c71e3b9fe533af2c168a36c7893be5&stat=instructions%3Au

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

This patch extends checkOrAndOpImpliedByOther to handle and/or trees.
Limitation: At least one of the operands of root and/or instruction should be an icmp. That is, this patch doesn't support expressions like (cmp1 & cmp2) & (cmp3 & cmp4). To handle this kind of pattern, please see #117118.

Closes #117107.
Compile-time impact: http://llvm-compile-time-tracker.com/compare.php?from=69cc3f096ccbdef526bbd5a065a25c95122e87ee&to=a90be3ac98e38415894bea1f2e68c475954b6ec3&stat=instructions%3Au

---

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

3 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/ConstraintElimination.cpp (+31-22)
• (modified) llvm/test/Transforms/ConstraintElimination/or.ll (+1-1)
• (added) llvm/test/Transforms/ConstraintElimination/unreachable-bb.ll (+36)

diff --git a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
index d03e3a0570cd3f..31e9691ecde4c7 100644
--- a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
@@ -1498,9 +1498,6 @@ static bool checkOrAndOpImpliedByOther(
     FactOrCheck &CB, ConstraintInfo &Info, Module *ReproducerModule,
     SmallVectorImpl<ReproducerEntry> &ReproducerCondStack,
     SmallVectorImpl<StackEntry> &DFSInStack) {
-
-  CmpInst::Predicate Pred;
-  Value *A, *B;
   Instruction *JoinOp = CB.getContextInst();
   CmpInst *CmpToCheck = cast<CmpInst>(CB.getInstructionToSimplify());
   unsigned OtherOpIdx = JoinOp->getOperand(0) == CmpToCheck ? 1 : 0;
@@ -1511,22 +1508,40 @@ static bool checkOrAndOpImpliedByOther(
   if (OtherOpIdx != 0 && isa<SelectInst>(JoinOp))
     return false;
 
-  if (!match(JoinOp->getOperand(OtherOpIdx),
-             m_ICmp(Pred, m_Value(A), m_Value(B))))
-    return false;
-
-  // For OR, check if the negated condition implies CmpToCheck.
-  bool IsOr = match(JoinOp, m_LogicalOr());
-  if (IsOr)
-    Pred = CmpInst::getInversePredicate(Pred);
-
   // Optimistically add fact from first condition.
   unsigned OldSize = DFSInStack.size();
-  Info.addFact(Pred, A, B, CB.NumIn, CB.NumOut, DFSInStack);
+  auto InfoRestorer = make_scope_exit([&]() {
+    // Remove entries again.
+    while (OldSize < DFSInStack.size()) {
+      StackEntry E = DFSInStack.back();
+      removeEntryFromStack(E, Info, ReproducerModule, ReproducerCondStack,
+                           DFSInStack);
+    }
+  });
+  // For OR, check if the negated condition implies CmpToCheck.
+  bool IsOr = match(JoinOp, m_LogicalOr());
+  SmallVector<Value *, 4> Worklist({JoinOp->getOperand(OtherOpIdx)});
+  while (!Worklist.empty()) {
+    Value *Val = Worklist.pop_back_val();
+    Value *LHS, *RHS;
+    ICmpInst::Predicate Pred;
+    if (match(Val, m_ICmp(Pred, m_Value(LHS), m_Value(RHS)))) {
+      if (IsOr)
+        Pred = CmpInst::getInversePredicate(Pred);
+      Info.addFact(Pred, LHS, RHS, CB.NumIn, CB.NumOut, DFSInStack);
+      continue;
+    }
+    if (IsOr ? match(Val, m_LogicalOr(m_Value(LHS), m_Value(RHS)))
+             : match(Val, m_LogicalAnd(m_Value(LHS), m_Value(RHS)))) {
+      Worklist.push_back(LHS);
+      Worklist.push_back(RHS);
+      continue;
+    }
+    return false;
+  }
   if (OldSize == DFSInStack.size())
     return false;
 
-  bool Changed = false;
   // Check if the second condition can be simplified now.
   if (auto ImpliedCondition =
           checkCondition(CmpToCheck->getPredicate(), CmpToCheck->getOperand(0),
@@ -1540,16 +1555,10 @@ static bool checkOrAndOpImpliedByOther(
           1 - OtherOpIdx,
           ConstantInt::getBool(JoinOp->getType(), *ImpliedCondition));
 
-    Changed = true;
+    return true;
   }
 
-  // Remove entries again.
-  while (OldSize < DFSInStack.size()) {
-    StackEntry E = DFSInStack.back();
-    removeEntryFromStack(E, Info, ReproducerModule, ReproducerCondStack,
-                         DFSInStack);
-  }
-  return Changed;
+  return false;
 }
 
 void ConstraintInfo::addFact(CmpInst::Predicate Pred, Value *A, Value *B,
diff --git a/llvm/test/Transforms/ConstraintElimination/or.ll b/llvm/test/Transforms/ConstraintElimination/or.ll
index 01b8ca973efa56..b401d6f1813695 100644
--- a/llvm/test/Transforms/ConstraintElimination/or.ll
+++ b/llvm/test/Transforms/ConstraintElimination/or.ll
@@ -124,7 +124,7 @@ define i1 @test_or_chain_ule_1(i4 %x, i4 %y, i4 %z, i4 %a, i4 %b) {
 ; CHECK-NEXT:    [[C_3:%.*]] = icmp ule i4 2, [[X]]
 ; CHECK-NEXT:    [[C_4:%.*]] = icmp ule i4 2, [[A:%.*]]
 ; CHECK-NEXT:    [[OR_1:%.*]] = or i1 [[C_1]], [[C_2]]
-; CHECK-NEXT:    [[OR_2:%.*]] = or i1 [[OR_1]], [[C_3]]
+; CHECK-NEXT:    [[OR_2:%.*]] = or i1 [[OR_1]], true
 ; CHECK-NEXT:    [[OR_3:%.*]] = or i1 [[C_4]], [[OR_2]]
 ; CHECK-NEXT:    br i1 [[OR_3]], label [[BB1:%.*]], label [[EXIT:%.*]]
 ; CHECK:       bb1:
diff --git a/llvm/test/Transforms/ConstraintElimination/unreachable-bb.ll b/llvm/test/Transforms/ConstraintElimination/unreachable-bb.ll
new file mode 100644
index 00000000000000..a3cea2e3b7889d
--- /dev/null
+++ b/llvm/test/Transforms/ConstraintElimination/unreachable-bb.ll
@@ -0,0 +1,36 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -p constraint-elimination -S %s | FileCheck %s
+
+define void @f(i32 noundef %v0, i32 noundef %v1, i32 noundef %v2) {
+; CHECK-LABEL: define void @f(
+; CHECK-SAME: i32 noundef [[V0:%.*]], i32 noundef [[V1:%.*]], i32 noundef [[V2:%.*]]) {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[CMP0:%.*]] = icmp sge i32 [[V0]], [[V1]]
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp sge i32 [[V1]], [[V2]]
+; CHECK-NEXT:    [[AND1:%.*]] = and i1 [[CMP0]], [[CMP1]]
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp slt i32 [[V0]], [[V2]]
+; CHECK-NEXT:    [[AND2:%.*]] = and i1 false, [[AND1]]
+; CHECK-NEXT:    br i1 [[AND2]], label %[[IF_THEN:.*]], label %[[RETURN:.*]]
+; CHECK:       [[IF_THEN]]:
+; CHECK-NEXT:    call void @side_effect()
+; CHECK-NEXT:    br label %[[RETURN]]
+; CHECK:       [[RETURN]]:
+; CHECK-NEXT:    ret void
+;
+entry:
+  %cmp0 = icmp sge i32 %v0, %v1
+  %cmp1 = icmp sge i32 %v1, %v2
+  %and1 = and i1 %cmp0, %cmp1
+  %cmp2 = icmp slt i32 %v0, %v2
+  %and2 = and i1 %cmp2, %and1
+  br i1 %and2, label %if.then, label %return
+
+if.then:
+  call void @side_effect()
+  br label %return
+
+return:
+  ret void
+}
+
+declare void @side_effect()

[nikic]

The implementation looks fine to me, but the test coverage here is really light.

Do we have a negative test for incorrect and/or mixing?

[regehr]

thanks for doing this compile-time study! that would have been my main worry. did you happen to check if this makes constraint elimination fire more in practice? do we have benchmarks (with UBSan turned on for example) that contain extensive opportunities for constraint elimination?

of course it's not a substitute for testing, but my ongoing search for missed optimizations is also a bug hunt, though it's probably the case that purpose-built fuzzers are better at that job.

[nikic]

@regehr You can see the impact on real-world code at dtcxzyw/llvm-opt-benchmark#1735.

[nikic]

LGTM

[fhahn]

LGTM, thanks!