[ValueTracking] Fix bug of using wrong condition for deducing KnownBits

[goldsteinn]

• [ValueTracking] Add test for issue 124275
• [ValueTracking] Fix bug of using wrong condition for deducing KnownBits

Fixes #124275

Bug was introduced by #114689

Now that computeKnownBits supports breaking out of recursive Phi
nodes, IncValue can be an operand of a different Phi than P. This
breaks the previous assumptions we had when using the possibly
condition at CxtI to constrain IncValue.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: None (goldsteinn)

Changes

• [ValueTracking] Add test for issue 124275
• [ValueTracking] Fix bug of using wrong condition for deducing KnownBits

Fixes #124275

Bug was introduced by #114689

Now that computeKnownBits supports breaking out of recursive Phi
nodes, IncValue can be an operand of a different Phi than P. This
breaks the previous assumptions we had when using the possibly
condition at CxtI to constrain IncValue.

---

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

2 Files Affected:

• (modified) llvm/lib/Analysis/ValueTracking.cpp (+12-5)
• (modified) llvm/test/Analysis/ValueTracking/phi-known-bits.ll (+38)

diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index eba728c7c8c360..477b934b122228 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -593,11 +593,14 @@ static bool cmpExcludesZero(CmpInst::Predicate Pred, const Value *RHS) {
 }
 
 static void breakSelfRecursivePHI(const Use *U, const PHINode *PHI,
-                                  Value *&ValOut, Instruction *&CtxIOut) {
+                                  Value *&ValOut, Instruction *&CtxIOut,
+                                  const PHINode **PhiOut = nullptr) {
   ValOut = U->get();
   if (ValOut == PHI)
     return;
   CtxIOut = PHI->getIncomingBlock(*U)->getTerminator();
+  if (PhiOut)
+    *PhiOut = PHI;
   Value *V;
   // If the Use is a select of this phi, compute analysis on other arm to break
   // recursion.
@@ -610,11 +613,13 @@ static void breakSelfRecursivePHI(const Use *U, const PHINode *PHI,
   // incoming value to break recursion.
   // TODO: We could handle any number of incoming edges as long as we only have
   // two unique values.
-  else if (auto *IncPhi = dyn_cast<PHINode>(ValOut);
+  if (auto *IncPhi = dyn_cast<PHINode>(ValOut);
            IncPhi && IncPhi->getNumIncomingValues() == 2) {
     for (int Idx = 0; Idx < 2; ++Idx) {
       if (IncPhi->getIncomingValue(Idx) == PHI) {
         ValOut = IncPhi->getIncomingValue(1 - Idx);
+        if (PhiOut)
+          *PhiOut = IncPhi;
         CtxIOut = IncPhi->getIncomingBlock(1 - Idx)->getTerminator();
         break;
       }
@@ -1673,8 +1678,9 @@ static void computeKnownBitsFromOperator(const Operator *I,
       Known.One.setAllBits();
       for (const Use &U : P->operands()) {
         Value *IncValue;
+        const PHINode *CxtPhi;
         Instruction *CxtI;
-        breakSelfRecursivePHI(&U, P, IncValue, CxtI);
+        breakSelfRecursivePHI(&U, P, IncValue, CxtI, &CxtPhi);
         // Skip direct self references.
         if (IncValue == P)
           continue;
@@ -1705,9 +1711,10 @@ static void computeKnownBitsFromOperator(const Operator *I,
                     m_Br(m_c_ICmp(Pred, m_Specific(IncValue), m_APInt(RHSC)),
                          m_BasicBlock(TrueSucc), m_BasicBlock(FalseSucc)))) {
             // Check for cases of duplicate successors.
-            if ((TrueSucc == P->getParent()) != (FalseSucc == P->getParent())) {
+            if ((TrueSucc == CxtPhi->getParent()) !=
+                (FalseSucc == CxtPhi->getParent())) {
               // If we're using the false successor, invert the predicate.
-              if (FalseSucc == P->getParent())
+              if (FalseSucc == CxtPhi->getParent())
                 Pred = CmpInst::getInversePredicate(Pred);
               // Get the knownbits implied by the incoming phi condition.
               auto CR = ConstantRange::makeExactICmpRegion(Pred, *RHSC);
diff --git a/llvm/test/Analysis/ValueTracking/phi-known-bits.ll b/llvm/test/Analysis/ValueTracking/phi-known-bits.ll
index 84220832f068f1..436aadbc25de6f 100644
--- a/llvm/test/Analysis/ValueTracking/phi-known-bits.ll
+++ b/llvm/test/Analysis/ValueTracking/phi-known-bits.ll
@@ -1112,3 +1112,41 @@ cleanup:
   %retval.0 = phi i1 [ %cmp, %if.then ], [ %tobool, %if.end4 ]
   ret i1 %retval.0
 }
+
+
+define i32 @issue_124275_wrong_br_direction(i32 noundef %inp) {
+; CHECK-LABEL: @issue_124275_wrong_br_direction(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = xor i32 [[INP:%.*]], -2
+; CHECK-NEXT:    [[XOR_INP_NEG:%.*]] = add i32 [[TMP0]], 1
+; CHECK-NEXT:    [[CMP_NE_NOT:%.*]] = icmp eq i32 [[XOR_INP_NEG]], 0
+; CHECK-NEXT:    br i1 [[CMP_NE_NOT]], label [[B1:%.*]], label [[B0:%.*]]
+; CHECK:       B0:
+; CHECK-NEXT:    [[PHI_B0:%.*]] = phi i32 [ [[PHI_B1:%.*]], [[B1]] ], [ [[XOR_INP_NEG]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    br label [[B1]]
+; CHECK:       B1:
+; CHECK-NEXT:    [[PHI_B1]] = phi i32 [ [[PHI_B0]], [[B0]] ], [ 0, [[ENTRY]] ]
+; CHECK-NEXT:    [[CMP_NE_B1_NOT:%.*]] = icmp eq i32 [[PHI_B1]], 0
+; CHECK-NEXT:    br i1 [[CMP_NE_B1_NOT]], label [[B0]], label [[END:%.*]]
+; CHECK:       end:
+; CHECK-NEXT:    ret i32 0
+;
+entry:
+  %xor_inp = xor i32 %inp, 1
+  %sub = sub i32 0, %xor_inp
+  %cmp_ne = icmp ne i32 %sub, 0
+  br i1 %cmp_ne, label %B0, label %B1
+
+B0:
+  %phi_B0 = phi i32 [ %phi_B1, %B1 ], [ %sub, %entry ]
+  br label %B1
+
+B1:
+  %phi_B1 = phi i32 [ %phi_B0, %B0 ], [ 0, %entry ]
+  %cmp_ne_B1 = icmp ne i32 %phi_B1, 0
+  %cmp_eq_B1 = xor i1 %cmp_ne_B1, true
+  br i1 %cmp_eq_B1, label %B0, label %end
+
+end:
+  ret i32 0
+}

[github-actions]

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

[nikic]

Can we fix this by replacing RecQ.CxtI with P->getIncomingBlock(u)->getTerminator() instead? (In terms of minimally ugly correct implementation, not maximally theoretically powerful.)

[goldsteinn]

I don't think so. Take the test case. If the branch in %B1 used a %cmp on %sub that would be patently incorrect to use when analyzing %sub in %B0

[nikic]

LGTM