Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 28, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be7ef32
[ValueTracking] Add test for issue 124275
goldsteinn Jan 26, 2025
4a16816
[ValueTracking] Fix bug of using wrong condition for deducing KnownBits
goldsteinn Jan 26, 2025
74ce859
Fix Fmt
goldsteinn Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions llvm/lib/Analysis/ValueTracking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
IncPhi && IncPhi->getNumIncomingValues() == 2) {
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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1705,9 +1711,10 @@ static void computeKnownBitsFromOperator(const Operator *I,
m_Br(m_c_ICmp(Pred, m_Specific(IncValue), m_APInt(RHSC)),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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);
Expand Down
38 changes: 38 additions & 0 deletions llvm/test/Analysis/ValueTracking/phi-known-bits.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}