Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ValueTracking] Expand cmpExcludesZero to optionally work with non-constant RHS #69364

Conversation

goldsteinn
Copy link
Contributor

  • [ValueTracking] Add tests for proving phi-nonzero based on ICmp without constants; NFC
  • [ValueTracking] Expand cmpExcludesZero to optionally work with non-constant RHS

@goldsteinn goldsteinn changed the title perf/goldsteinn/cmp excludes zero non const [ValueTracking] Expand cmpExcludesZero to optionally work with non-constant RHS Oct 17, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 17, 2023

@llvm/pr-subscribers-llvm-analysis

Author: None (goldsteinn)

Changes
  • [ValueTracking] Add tests for proving phi-nonzero based on ICmp without constants; NFC
  • [ValueTracking] Expand cmpExcludesZero to optionally work with non-constant RHS

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

3 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+56-22)
  • (modified) llvm/test/Analysis/ValueTracking/phi-known-nonzero.ll (+198)
  • (modified) llvm/test/Analysis/ValueTracking/select-known-non-zero.ll (+13-52)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 1e0281b3f1bd79e..6a92f2247b46f58 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -552,7 +552,9 @@ bool llvm::isValidAssumeForContext(const Instruction *Inv,
 // example Pred=EQ, RHS=isKnownNonZero. cmpExcludesZero is called in loops
 // so the extra compile time may not be worth it, but possibly a second API
 // should be created for use outside of loops.
-static bool cmpExcludesZero(CmpInst::Predicate Pred, const Value *RHS) {
+static bool cmpExcludesZero(CmpInst::Predicate Pred, Value *RHS,
+                            const SimplifyQuery *Q = nullptr,
+                            unsigned Depth = 0) {
   // v u> y implies v != 0.
   if (Pred == ICmpInst::ICMP_UGT)
     return true;
@@ -562,25 +564,44 @@ static bool cmpExcludesZero(CmpInst::Predicate Pred, const Value *RHS) {
     return match(RHS, m_Zero());
 
   // All other predicates - rely on generic ConstantRange handling.
+
   const APInt *C;
-  auto Zero = APInt::getZero(RHS->getType()->getScalarSizeInBits());
   if (match(RHS, m_APInt(C))) {
     ConstantRange TrueValues = ConstantRange::makeExactICmpRegion(Pred, *C);
-    return !TrueValues.contains(Zero);
+    return !TrueValues.contains(APInt::getZero(C->getBitWidth()));
   }
 
-  auto *VC = dyn_cast<ConstantDataVector>(RHS);
-  if (VC == nullptr)
-    return false;
+  auto *FVTy = dyn_cast<FixedVectorType>(RHS->getType());
+  Constant *VC;
+  if (FVTy != nullptr && match(RHS, m_ImmConstant(VC))) {
+    unsigned EleIdx, NEle;
+    for (EleIdx = 0, NEle = FVTy->getNumElements(); EleIdx < NEle; ++EleIdx) {
+      Constant *EleC = VC->getAggregateElement(EleIdx);
+      if (EleC == nullptr)
+       break;
+      const APInt *EleCI;
+      if (!match(EleC, m_APInt(EleCI)))
+       break;
+      ConstantRange TrueValues =
+          ConstantRange::makeExactICmpRegion(Pred, *EleCI);
+      if (TrueValues.contains(APInt::getZero(EleCI->getBitWidth())))
+       break;
+    }
+    if (EleIdx == NEle)
+      return true;
+  }
 
-  for (unsigned ElemIdx = 0, NElem = VC->getNumElements(); ElemIdx < NElem;
-       ++ElemIdx) {
-    ConstantRange TrueValues = ConstantRange::makeExactICmpRegion(
-        Pred, VC->getElementAsAPInt(ElemIdx));
-    if (TrueValues.contains(Zero))
-      return false;
+  if (Q != nullptr && RHS->getType()->isIntOrIntVectorTy()) {
+    ConstantRange TrueValues = ConstantRange::makeAllowedICmpRegion(
+        Pred,
+        computeConstantRange(RHS, ICmpInst::isSigned(Pred), Q->IIQ.UseInstrInfo,
+                             Q->AC, Q->CxtI, Q->DT, Depth));
+    if (!TrueValues.contains(
+            APInt::getZero(RHS->getType()->getScalarSizeInBits())))
+      return true;
   }
-  return true;
+
+  return false;
 }
 
 static bool isKnownNonZeroFromAssume(const Value *V, const SimplifyQuery &Q) {
@@ -2666,15 +2687,21 @@ static bool isKnownNonZeroFromOperator(const Operator *I,
 
       // The condition of the select dominates the true/false arm. Check if the
       // condition implies that a given arm is non-zero.
-      Value *X;
+      Value *X, *Y;
       CmpInst::Predicate Pred;
-      if (!match(I->getOperand(0), m_c_ICmp(Pred, m_Specific(Op), m_Value(X))))
+      if (!match(I->getOperand(0), m_ICmp(Pred, m_Value(X), m_Value(Y))))
+        return false;
+      if (Y == Op) {
+        Pred = ICmpInst::getSwappedPredicate(Pred);
+        std::swap(X, Y);
+      }
+      if (X != Op)
         return false;
 
       if (!IsTrueArm)
         Pred = ICmpInst::getInversePredicate(Pred);
 
-      return cmpExcludesZero(Pred, X);
+      return cmpExcludesZero(Pred, Y, &Q, Depth);
     };
 
     if (SelectArmIsNonZero(/* IsTrueArm */ true) &&
@@ -2696,18 +2723,25 @@ static bool isKnownNonZeroFromOperator(const Operator *I,
       RecQ.CxtI = PN->getIncomingBlock(U)->getTerminator();
       // Check if the branch on the phi excludes zero.
       ICmpInst::Predicate Pred;
-      Value *X;
+      Value *X, *Y;
       BasicBlock *TrueSucc, *FalseSucc;
       if (match(RecQ.CxtI,
-                m_Br(m_c_ICmp(Pred, m_Specific(U.get()), m_Value(X)),
+                m_Br(m_ICmp(Pred, m_Value(X), m_Value(Y)),
                      m_BasicBlock(TrueSucc), m_BasicBlock(FalseSucc)))) {
         // Check for cases of duplicate successors.
         if ((TrueSucc == PN->getParent()) != (FalseSucc == PN->getParent())) {
           // If we're using the false successor, invert the predicate.
-          if (FalseSucc == PN->getParent())
-            Pred = CmpInst::getInversePredicate(Pred);
-          if (cmpExcludesZero(Pred, X))
-            return true;
+
+          if (Y == U.get()) {
+            Pred = ICmpInst::getSwappedPredicate(Pred);
+            std::swap(X, Y);
+          }
+          if (X == U.get()) {
+            if (FalseSucc == PN->getParent())
+              Pred = CmpInst::getInversePredicate(Pred);
+            if (cmpExcludesZero(Pred, Y, &Q, NewDepth))
+              return true;
+          }
         }
       }
       // Finally recurse on the edge and check it directly.
diff --git a/llvm/test/Analysis/ValueTracking/phi-known-nonzero.ll b/llvm/test/Analysis/ValueTracking/phi-known-nonzero.ll
index d80d1704c73d1de..a45b51175466257 100644
--- a/llvm/test/Analysis/ValueTracking/phi-known-nonzero.ll
+++ b/llvm/test/Analysis/ValueTracking/phi-known-nonzero.ll
@@ -262,3 +262,201 @@ T:
 F:
   br label %T
 }
+
+define i1 @phi_uge_non_zero_non_const(i8 %x, i8 %y) {
+; CHECK-LABEL: @phi_uge_non_zero_non_const(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[YY:%.*]] = add nuw i8 [[Y:%.*]], 32
+; CHECK-NEXT:    [[CMP:%.*]] = icmp uge i8 [[X:%.*]], [[YY]]
+; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
+; CHECK:       T:
+; CHECK-NEXT:    ret i1 false
+; CHECK:       F:
+; CHECK-NEXT:    br label [[T]]
+;
+entry:
+  %yy = add nuw i8 %y, 32
+  %cmp = icmp uge i8 %x, %yy
+  br i1 %cmp, label %T, label %F
+T:
+  %v = phi i8 [ %x, %entry], [-1, %F]
+  %r = icmp eq i8 %v, 0
+  ret i1 %r
+F:
+  br label %T
+}
+
+define i1 @phi_uge_non_zero_non_const_fail(i8 %x, i8 %y) {
+; CHECK-LABEL: @phi_uge_non_zero_non_const_fail(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[YY:%.*]] = add i8 [[Y:%.*]], 32
+; CHECK-NEXT:    [[CMP:%.*]] = icmp uge i8 [[X:%.*]], [[YY]]
+; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
+; CHECK:       T:
+; CHECK-NEXT:    [[V:%.*]] = phi i8 [ [[X]], [[ENTRY:%.*]] ], [ -1, [[F]] ]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[V]], 0
+; CHECK-NEXT:    ret i1 [[R]]
+; CHECK:       F:
+; CHECK-NEXT:    br label [[T]]
+;
+entry:
+  %yy = add i8 %y, 32
+  %cmp = icmp uge i8 %x, %yy
+  br i1 %cmp, label %T, label %F
+T:
+  %v = phi i8 [ %x, %entry], [-1, %F]
+  %r = icmp eq i8 %v, 0
+  ret i1 %r
+F:
+  br label %T
+}
+
+define i1 @phi_slt_non_zero_non_const(i8 %x, i8 %y) {
+; CHECK-LABEL: @phi_slt_non_zero_non_const(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[YY:%.*]] = and i8 [[Y:%.*]], 95
+; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i8 [[YY]], [[X:%.*]]
+; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
+; CHECK:       T:
+; CHECK-NEXT:    ret i1 false
+; CHECK:       F:
+; CHECK-NEXT:    br label [[T]]
+;
+entry:
+  %yy = and i8 %y, 95
+  %cmp = icmp slt i8 %yy, %x
+  br i1 %cmp, label %T, label %F
+T:
+  %v = phi i8 [ %x, %entry], [-1, %F]
+  %r = icmp eq i8 %v, 0
+  ret i1 %r
+F:
+  br label %T
+}
+
+define i1 @phi_slt_non_zero_non_const_fail(i8 %x, i8 %y) {
+; CHECK-LABEL: @phi_slt_non_zero_non_const_fail(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[YY:%.*]] = or i8 [[Y:%.*]], 32
+; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i8 [[YY]], [[X:%.*]]
+; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
+; CHECK:       T:
+; CHECK-NEXT:    [[V:%.*]] = phi i8 [ [[X]], [[ENTRY:%.*]] ], [ -1, [[F]] ]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[V]], 0
+; CHECK-NEXT:    ret i1 [[R]]
+; CHECK:       F:
+; CHECK-NEXT:    br label [[T]]
+;
+entry:
+  %yy = or i8 %y, 32
+  %cmp = icmp slt i8 %yy, %x
+  br i1 %cmp, label %T, label %F
+T:
+  %v = phi i8 [ %x, %entry], [-1, %F]
+  %r = icmp eq i8 %v, 0
+  ret i1 %r
+F:
+  br label %T
+}
+
+define i1 @phi_sle_non_zero_non_const(i8 %x, i8 %y) {
+; CHECK-LABEL: @phi_sle_non_zero_non_const(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[YYY:%.*]] = and i8 [[Y:%.*]], 95
+; CHECK-NEXT:    [[YY:%.*]] = add nuw i8 [[YYY]], 4
+; CHECK-NEXT:    [[CMP:%.*]] = icmp sle i8 [[X:%.*]], [[YY]]
+; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
+; CHECK:       T:
+; CHECK-NEXT:    br label [[F]]
+; CHECK:       F:
+; CHECK-NEXT:    [[V:%.*]] = phi i8 [ [[X]], [[ENTRY:%.*]] ], [ -1, [[T]] ]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[V]], 0
+; CHECK-NEXT:    ret i1 [[R]]
+;
+entry:
+  %yyy = and i8 %y, 95
+  %yy = add nuw i8 %yyy, 4
+  %cmp = icmp sle i8 %x, %yy
+  br i1 %cmp, label %T, label %F
+T:
+  br label %F
+F:
+  %v = phi i8 [ %x, %entry], [-1, %T]
+  %r = icmp eq i8 %v, 0
+  ret i1 %r
+}
+
+define i1 @phi_sle_non_zero_non_const_fail(i8 %x, i8 %y) {
+; CHECK-LABEL: @phi_sle_non_zero_non_const_fail(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[YYY:%.*]] = and i8 [[Y:%.*]], 95
+; CHECK-NEXT:    [[YY:%.*]] = add nuw i8 [[YYY]], 4
+; CHECK-NEXT:    [[CMP:%.*]] = icmp sle i8 [[YY]], [[X:%.*]]
+; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
+; CHECK:       T:
+; CHECK-NEXT:    br label [[F]]
+; CHECK:       F:
+; CHECK-NEXT:    [[V:%.*]] = phi i8 [ [[X]], [[ENTRY:%.*]] ], [ -1, [[T]] ]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[V]], 0
+; CHECK-NEXT:    ret i1 [[R]]
+;
+entry:
+  %yyy = and i8 %y, 95
+  %yy = add nuw i8 %yyy, 4
+  %cmp = icmp sle i8 %yy, %x
+  br i1 %cmp, label %T, label %F
+T:
+  br label %F
+F:
+  %v = phi i8 [ %x, %entry], [-1, %T]
+  %r = icmp eq i8 %v, 0
+  ret i1 %r
+}
+
+define i1 @phi_eq_non_zero_non_const(i8 %x, i8 %y) {
+; CHECK-LABEL: @phi_eq_non_zero_non_const(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[YY:%.*]] = or i8 [[Y:%.*]], 1
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8 [[X:%.*]], [[YY]]
+; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
+; CHECK:       T:
+; CHECK-NEXT:    ret i1 false
+; CHECK:       F:
+; CHECK-NEXT:    br label [[T]]
+;
+entry:
+  %yy = or i8 %y, 1
+  %cmp = icmp eq i8 %x, %yy
+  br i1 %cmp, label %T, label %F
+T:
+  %v = phi i8 [ %x, %entry], [-1, %F]
+  %r = icmp eq i8 %v, 0
+  ret i1 %r
+F:
+  br label %T
+}
+
+define i1 @phi_eq_non_zero_non_const_fail(i8 %x, i8 %y) {
+; CHECK-LABEL: @phi_eq_non_zero_non_const_fail(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[YY:%.*]] = or i8 [[Y:%.*]], 1
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8 [[X:%.*]], [[YY]]
+; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
+; CHECK:       T:
+; CHECK-NEXT:    br label [[F]]
+; CHECK:       F:
+; CHECK-NEXT:    [[V:%.*]] = phi i8 [ [[X]], [[ENTRY:%.*]] ], [ -1, [[T]] ]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[V]], 0
+; CHECK-NEXT:    ret i1 [[R]]
+;
+entry:
+  %yy = or i8 %y, 1
+  %cmp = icmp eq i8 %x, %yy
+  br i1 %cmp, label %T, label %F
+T:
+  br label %F
+F:
+  %v = phi i8 [ %x, %entry], [-1, %T]
+  %r = icmp eq i8 %v, 0
+  ret i1 %r
+}
diff --git a/llvm/test/Analysis/ValueTracking/select-known-non-zero.ll b/llvm/test/Analysis/ValueTracking/select-known-non-zero.ll
index 8b1d2fd0181d66c..8240bcd80750066 100644
--- a/llvm/test/Analysis/ValueTracking/select-known-non-zero.ll
+++ b/llvm/test/Analysis/ValueTracking/select-known-non-zero.ll
@@ -26,10 +26,7 @@ define i1 @select_v_eq_nz(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp ne i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8 [[C]], [[V:%.*]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[V]], i8 [[Y]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)
@@ -47,10 +44,7 @@ define i1 @inv_select_v_ugt_nz(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp ne i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp ugt i8 [[C]], [[V:%.*]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[Y]], i8 [[V]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)
@@ -89,10 +83,7 @@ define i1 @select_v_uge_nz(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp ne i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp uge i8 [[V:%.*]], [[C]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[V]], i8 [[Y]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)
@@ -110,10 +101,7 @@ define i1 @select_v_sgt_nonneg(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp sge i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt i8 [[V:%.*]], [[C]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[V]], i8 [[Y]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)
@@ -152,10 +140,7 @@ define i1 @inv_select_v_sgt_neg(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp slt i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt i8 [[V:%.*]], [[C]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[Y]], i8 [[V]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)
@@ -173,10 +158,7 @@ define i1 @inv_select_v_sgt_nonneg_nz(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp sgt i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt i8 [[C]], [[V:%.*]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[Y]], i8 [[V]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)
@@ -194,10 +176,7 @@ define i1 @select_v_slt_nonneg(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp sge i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i8 [[C]], [[V:%.*]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[V]], i8 [[Y]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)
@@ -236,10 +215,7 @@ define i1 @select_v_slt_neg(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp slt i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i8 [[V:%.*]], [[C]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[V]], i8 [[Y]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)
@@ -257,10 +233,7 @@ define i1 @select_v_sge_nonneg_nz(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp sgt i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp sge i8 [[V:%.*]], [[C]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[V]], i8 [[Y]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)
@@ -278,10 +251,7 @@ define i1 @select_v_sge_neg(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp slt i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp sge i8 [[C]], [[V:%.*]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[V]], i8 [[Y]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)
@@ -316,10 +286,7 @@ define i1 @select_v_sle_neg(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp slt i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp sle i8 [[V:%.*]], [[C]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[V]], i8 [[Y]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)
@@ -337,10 +304,7 @@ define i1 @select_v_sle_nonneg_nz(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp sgt i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp sle i8 [[C]], [[V:%.*]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[V]], i8 [[Y]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)
@@ -379,10 +343,7 @@ define i1 @inv_select_v_sle_nonneg(i8 %v, i8 %C, i8 %y) {
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[YNZ]])
 ; CHECK-NEXT:    [[PCOND0:%.*]] = icmp sge i8 [[C:%.*]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[PCOND0]])
-; CHECK-NEXT:    [[CMP:%.*]] = icmp sle i8 [[V:%.*]], [[C]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 [[Y]], i8 [[V]]
-; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[S]], 0
-; CHECK-NEXT:    ret i1 [[R]]
+; CHECK-NEXT:    ret i1 false
 ;
   %ynz = icmp ne i8 %y, 0
   call void @llvm.assume(i1 %ynz)

…constant RHS

At the moment `cmpExcludesZero` is only useful if the RHS operand is
constant.

This is to keep the cost of the function low, as its used in some
relatively tight loops (like for analyzing AssumptionCache or
Dominating conditions).

There are other uses of `cmpExcludesZero`, however, that are less time
sensitive and can both benefit from and afford to do more
thorough analysis on non-constant operands.

This patch makes it so that `cmpExcludesZero` can optionally take a
`SimplifyQuery`, and if its provided, will compute a
`ConstantRange` for non-constant operands to check if zero is
excluded.
@goldsteinn goldsteinn force-pushed the perf/goldsteinn/cmp-excludes-zero-non-const branch from e768cd0 to 9031c83 Compare October 17, 2023 18:14
@github-actions
Copy link

github-actions bot commented Oct 17, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff b33723710f5194080e8bfab9f21c8445647c976b 9031c8347dd041a4f34d8499d9e842ca9c33fd27 -- llvm/lib/Analysis/ValueTracking.cpp
View the diff from clang-format here.
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index de22ce76ce45..a4f7ac6b1500 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -580,7 +580,7 @@ static bool cmpExcludesZero(CmpInst::Predicate Pred, Value *RHS,
       ConstantRange TrueValues = ConstantRange::makeExactICmpRegion(
           Pred, VC->getElementAsAPInt(ElemIdx));
       if (TrueValues.contains(Zero))
-       return false;
+        return false;
     }
     return true;
   }

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please provide the original motivating case for this? I think this goes beyond what is reasonable to handle in ValueTracking, and it would be best to handle your use case via LVI+CVP instead, if this is possible.

@goldsteinn
Copy link
Contributor Author

Could you please provide the original motivating case for this? I think this goes beyond what is reasonable to handle in ValueTracking, and it would be best to handle your use case via LVI+CVP instead, if this is possible.

No strong motivating force. Guess the thinking was this is still basically a cheap check:
https://llvm-compile-time-tracker.com/compare.php?from=0ec39fb48e9dc4aad5f20bc50d44947ac004dd34&to=9031c8347dd041a4f34d8499d9e842ca9c33fd27&stat=instructions:u
And just improves coverage.
Can try adding to LVI, however.

@@ -562,25 +564,37 @@ static bool cmpExcludesZero(CmpInst::Predicate Pred, const Value *RHS) {
return match(RHS, m_Zero());

// All other predicates - rely on generic ConstantRange handling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the newline here.

@@ -552,7 +552,9 @@ bool llvm::isValidAssumeForContext(const Instruction *Inv,
// example Pred=EQ, RHS=isKnownNonZero. cmpExcludesZero is called in loops
// so the extra compile time may not be worth it, but possibly a second API
// should be created for use outside of loops.
static bool cmpExcludesZero(CmpInst::Predicate Pred, const Value *RHS) {
static bool cmpExcludesZero(CmpInst::Predicate Pred, Value *RHS,
const SimplifyQuery *Q = nullptr,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const SimplifyQuery *Q = nullptr,
const SimplifyQuery &Q,

It is strange to me. I have checked all callers of cmpExcludeZero and found that SimplifyQuery is always available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its intentional so we can still have the const-only version. the perf cost of doing recursive CR call in the assume loop for example would be significant.

CmpInst::Predicate Pred;
if (!match(I->getOperand(0), m_c_ICmp(Pred, m_Specific(Op), m_Value(X))))
if (!match(I->getOperand(0), m_ICmp(Pred, m_Value(X), m_Value(Y))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like a NFC change. Does this change improve matching performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its necessary now that we can use non-constants. I'll seperate that to an NFC and rebase this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will probably abandon this one per nikic's comments, but will update the matching logics

BasicBlock *TrueSucc, *FalseSucc;
if (match(RecQ.CxtI,
m_Br(m_c_ICmp(Pred, m_Specific(U.get()), m_Value(X)),
m_Br(m_ICmp(Pred, m_Value(X), m_Value(Y)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is this one.

@goldsteinn goldsteinn closed this Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants