From be7ef32128fe7b81b69f5c8b8897eeec22ed191f Mon Sep 17 00:00:00 2001 From: Noah Goldstein Date: Sun, 26 Jan 2025 14:18:06 -0600 Subject: [PATCH 1/3] [ValueTracking] Add test for issue 124275 --- .../Analysis/ValueTracking/phi-known-bits.ll | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/llvm/test/Analysis/ValueTracking/phi-known-bits.ll b/llvm/test/Analysis/ValueTracking/phi-known-bits.ll index 84220832f068f..e9bbc124ff3c4 100644 --- a/llvm/test/Analysis/ValueTracking/phi-known-bits.ll +++ b/llvm/test/Analysis/ValueTracking/phi-known-bits.ll @@ -1112,3 +1112,36 @@ 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: [[CMP_NE_NOT:%.*]] = icmp eq i32 [[INP:%.*]], 1 +; CHECK-NEXT: br i1 [[CMP_NE_NOT]], label [[B1:%.*]], label [[B0:%.*]] +; CHECK: B0: +; CHECK-NEXT: br label [[B1]] +; CHECK: B1: +; CHECK-NEXT: br i1 true, 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 +} From 4a168168abbbd23299a1ddd043fb3a34cf1fa4fb Mon Sep 17 00:00:00 2001 From: Noah Goldstein Date: Sun, 26 Jan 2025 14:30:39 -0600 Subject: [PATCH 2/3] [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`. --- llvm/lib/Analysis/ValueTracking.cpp | 17 ++++++++++++----- .../Analysis/ValueTracking/phi-known-bits.ll | 9 +++++++-- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp index eba728c7c8c36..477b934b12222 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(ValOut); + if (auto *IncPhi = dyn_cast(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 e9bbc124ff3c4..436aadbc25de6 100644 --- a/llvm/test/Analysis/ValueTracking/phi-known-bits.ll +++ b/llvm/test/Analysis/ValueTracking/phi-known-bits.ll @@ -1117,12 +1117,17 @@ cleanup: define i32 @issue_124275_wrong_br_direction(i32 noundef %inp) { ; CHECK-LABEL: @issue_124275_wrong_br_direction( ; CHECK-NEXT: entry: -; CHECK-NEXT: [[CMP_NE_NOT:%.*]] = icmp eq i32 [[INP:%.*]], 1 +; 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: br i1 true, label [[B0]], label [[END:%.*]] +; 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 ; From 74ce859b461013b407f4ec95a03bf53c82791599 Mon Sep 17 00:00:00 2001 From: Noah Goldstein Date: Tue, 28 Jan 2025 11:17:00 -0600 Subject: [PATCH 3/3] Fix Fmt --- llvm/lib/Analysis/ValueTracking.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp index 477b934b12222..b63a0a07f7de2 100644 --- a/llvm/lib/Analysis/ValueTracking.cpp +++ b/llvm/lib/Analysis/ValueTracking.cpp @@ -614,7 +614,7 @@ static void breakSelfRecursivePHI(const Use *U, const PHINode *PHI, // TODO: We could handle any number of incoming edges as long as we only have // two unique values. if (auto *IncPhi = dyn_cast(ValOut); - IncPhi && IncPhi->getNumIncomingValues() == 2) { + IncPhi && IncPhi->getNumIncomingValues() == 2) { for (int Idx = 0; Idx < 2; ++Idx) { if (IncPhi->getIncomingValue(Idx) == PHI) { ValOut = IncPhi->getIncomingValue(1 - Idx);