Skip to content

Commit c2fba02

Browse files
authored
[ValueTracking] Fix bug of using wrong condition for deducing KnownBits (#124481)
- **[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`.
1 parent 35df525 commit c2fba02

File tree

2 files changed

+51
-6
lines changed

2 files changed

+51
-6
lines changed

llvm/lib/Analysis/ValueTracking.cpp

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -593,11 +593,14 @@ static bool cmpExcludesZero(CmpInst::Predicate Pred, const Value *RHS) {
593593
}
594594

595595
static void breakSelfRecursivePHI(const Use *U, const PHINode *PHI,
596-
Value *&ValOut, Instruction *&CtxIOut) {
596+
Value *&ValOut, Instruction *&CtxIOut,
597+
const PHINode **PhiOut = nullptr) {
597598
ValOut = U->get();
598599
if (ValOut == PHI)
599600
return;
600601
CtxIOut = PHI->getIncomingBlock(*U)->getTerminator();
602+
if (PhiOut)
603+
*PhiOut = PHI;
601604
Value *V;
602605
// If the Use is a select of this phi, compute analysis on other arm to break
603606
// recursion.
@@ -610,11 +613,13 @@ static void breakSelfRecursivePHI(const Use *U, const PHINode *PHI,
610613
// incoming value to break recursion.
611614
// TODO: We could handle any number of incoming edges as long as we only have
612615
// two unique values.
613-
else if (auto *IncPhi = dyn_cast<PHINode>(ValOut);
614-
IncPhi && IncPhi->getNumIncomingValues() == 2) {
616+
if (auto *IncPhi = dyn_cast<PHINode>(ValOut);
617+
IncPhi && IncPhi->getNumIncomingValues() == 2) {
615618
for (int Idx = 0; Idx < 2; ++Idx) {
616619
if (IncPhi->getIncomingValue(Idx) == PHI) {
617620
ValOut = IncPhi->getIncomingValue(1 - Idx);
621+
if (PhiOut)
622+
*PhiOut = IncPhi;
618623
CtxIOut = IncPhi->getIncomingBlock(1 - Idx)->getTerminator();
619624
break;
620625
}
@@ -1673,8 +1678,9 @@ static void computeKnownBitsFromOperator(const Operator *I,
16731678
Known.One.setAllBits();
16741679
for (const Use &U : P->operands()) {
16751680
Value *IncValue;
1681+
const PHINode *CxtPhi;
16761682
Instruction *CxtI;
1677-
breakSelfRecursivePHI(&U, P, IncValue, CxtI);
1683+
breakSelfRecursivePHI(&U, P, IncValue, CxtI, &CxtPhi);
16781684
// Skip direct self references.
16791685
if (IncValue == P)
16801686
continue;
@@ -1705,9 +1711,10 @@ static void computeKnownBitsFromOperator(const Operator *I,
17051711
m_Br(m_c_ICmp(Pred, m_Specific(IncValue), m_APInt(RHSC)),
17061712
m_BasicBlock(TrueSucc), m_BasicBlock(FalseSucc)))) {
17071713
// Check for cases of duplicate successors.
1708-
if ((TrueSucc == P->getParent()) != (FalseSucc == P->getParent())) {
1714+
if ((TrueSucc == CxtPhi->getParent()) !=
1715+
(FalseSucc == CxtPhi->getParent())) {
17091716
// If we're using the false successor, invert the predicate.
1710-
if (FalseSucc == P->getParent())
1717+
if (FalseSucc == CxtPhi->getParent())
17111718
Pred = CmpInst::getInversePredicate(Pred);
17121719
// Get the knownbits implied by the incoming phi condition.
17131720
auto CR = ConstantRange::makeExactICmpRegion(Pred, *RHSC);

llvm/test/Analysis/ValueTracking/phi-known-bits.ll

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1112,3 +1112,41 @@ cleanup:
11121112
%retval.0 = phi i1 [ %cmp, %if.then ], [ %tobool, %if.end4 ]
11131113
ret i1 %retval.0
11141114
}
1115+
1116+
1117+
define i32 @issue_124275_wrong_br_direction(i32 noundef %inp) {
1118+
; CHECK-LABEL: @issue_124275_wrong_br_direction(
1119+
; CHECK-NEXT: entry:
1120+
; CHECK-NEXT: [[TMP0:%.*]] = xor i32 [[INP:%.*]], -2
1121+
; CHECK-NEXT: [[XOR_INP_NEG:%.*]] = add i32 [[TMP0]], 1
1122+
; CHECK-NEXT: [[CMP_NE_NOT:%.*]] = icmp eq i32 [[XOR_INP_NEG]], 0
1123+
; CHECK-NEXT: br i1 [[CMP_NE_NOT]], label [[B1:%.*]], label [[B0:%.*]]
1124+
; CHECK: B0:
1125+
; CHECK-NEXT: [[PHI_B0:%.*]] = phi i32 [ [[PHI_B1:%.*]], [[B1]] ], [ [[XOR_INP_NEG]], [[ENTRY:%.*]] ]
1126+
; CHECK-NEXT: br label [[B1]]
1127+
; CHECK: B1:
1128+
; CHECK-NEXT: [[PHI_B1]] = phi i32 [ [[PHI_B0]], [[B0]] ], [ 0, [[ENTRY]] ]
1129+
; CHECK-NEXT: [[CMP_NE_B1_NOT:%.*]] = icmp eq i32 [[PHI_B1]], 0
1130+
; CHECK-NEXT: br i1 [[CMP_NE_B1_NOT]], label [[B0]], label [[END:%.*]]
1131+
; CHECK: end:
1132+
; CHECK-NEXT: ret i32 0
1133+
;
1134+
entry:
1135+
%xor_inp = xor i32 %inp, 1
1136+
%sub = sub i32 0, %xor_inp
1137+
%cmp_ne = icmp ne i32 %sub, 0
1138+
br i1 %cmp_ne, label %B0, label %B1
1139+
1140+
B0:
1141+
%phi_B0 = phi i32 [ %phi_B1, %B1 ], [ %sub, %entry ]
1142+
br label %B1
1143+
1144+
B1:
1145+
%phi_B1 = phi i32 [ %phi_B0, %B0 ], [ 0, %entry ]
1146+
%cmp_ne_B1 = icmp ne i32 %phi_B1, 0
1147+
%cmp_eq_B1 = xor i1 %cmp_ne_B1, true
1148+
br i1 %cmp_eq_B1, label %B0, label %end
1149+
1150+
end:
1151+
ret i32 0
1152+
}

0 commit comments

Comments
 (0)