Skip to content

[ValueTracking] Use SimplifyQuery in isKnownNonEqual #124942

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

Merged
merged 2 commits into from
Feb 1, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 2 additions & 5 deletions llvm/include/llvm/Analysis/ValueTracking.h
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,8 @@ bool isKnownNegative(const Value *V, const SimplifyQuery &SQ,

/// Return true if the given values are known to be non-equal when defined.
/// Supports scalar integer types only.
bool isKnownNonEqual(const Value *V1, const Value *V2, const DataLayout &DL,
AssumptionCache *AC = nullptr,
const Instruction *CxtI = nullptr,
const DominatorTree *DT = nullptr,
bool UseInstrInfo = true);
bool isKnownNonEqual(const Value *V1, const Value *V2, const SimplifyQuery &SQ,
unsigned Depth = 0);

/// Return true if 'V & Mask' is known to be zero. We use this predicate to
/// simplify operations downstream. Mask is known to be zero for bits that V
Expand Down
6 changes: 4 additions & 2 deletions llvm/lib/Analysis/BasicAliasAnalysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1349,8 +1349,10 @@ AliasResult BasicAAResult::aliasGEP(
const VariableGEPIndex &Var1 = DecompGEP1.VarIndices[1];
if (Var0.hasNegatedScaleOf(Var1) && Var0.Val.TruncBits == 0 &&
Var0.Val.hasSameCastsAs(Var1.Val) && !AAQI.MayBeCrossIteration &&
isKnownNonEqual(Var0.Val.V, Var1.Val.V, DL, &AC, /* CxtI */ nullptr,
DT))
isKnownNonEqual(Var0.Val.V, Var1.Val.V,
SimplifyQuery(DL, DT, &AC, /*CxtI=*/Var0.CxtI
? Var0.CxtI
: Var1.CxtI)))
Copy link
Contributor

Choose a reason for hiding this comment

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

This was actually using a null context instruction intentionally, to avoid order-dependence. But due to the implicit context, it ended up not mattering, and now things rely on it... It's okay for now, keeping symmetry in this code is not super important.

MinAbsVarIndex = Var0.Scale.abs();
}

Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Analysis/InstructionSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3994,7 +3994,7 @@ static Value *simplifyICmpInst(CmpPredicate Pred, Value *LHS, Value *RHS,
// This is potentially expensive, and we have already computedKnownBits for
// compares with 0 above here, so only try this for a non-zero compare.
if (ICmpInst::isEquality(Pred) && !match(RHS, m_Zero()) &&
isKnownNonEqual(LHS, RHS, Q.DL, Q.AC, Q.CxtI, Q.DT, Q.IIQ.UseInstrInfo)) {
isKnownNonEqual(LHS, RHS, Q)) {
return Pred == ICmpInst::ICMP_NE ? getTrue(ITy) : getFalse(ITy);
}

Expand Down
8 changes: 2 additions & 6 deletions llvm/lib/Analysis/ValueTracking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -316,18 +316,14 @@ static bool isKnownNonEqual(const Value *V1, const Value *V2,
const SimplifyQuery &Q);

bool llvm::isKnownNonEqual(const Value *V1, const Value *V2,
const DataLayout &DL, AssumptionCache *AC,
const Instruction *CxtI, const DominatorTree *DT,
bool UseInstrInfo) {
const SimplifyQuery &Q, unsigned Depth) {
// We don't support looking through casts.
if (V1 == V2 || V1->getType() != V2->getType())
return false;
auto *FVTy = dyn_cast<FixedVectorType>(V1->getType());
APInt DemandedElts =
FVTy ? APInt::getAllOnes(FVTy->getNumElements()) : APInt(1, 1);
return ::isKnownNonEqual(
V1, V2, DemandedElts, 0,
SimplifyQuery(DL, DT, AC, safeCxtI(V2, V1, CxtI), UseInstrInfo));
return ::isKnownNonEqual(V1, V2, DemandedElts, Depth, Q);
Comment on lines -328 to +326
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you add the Depth argument? Is the plan to support depth > 0 in follow-ups?

Copy link
Member Author

Choose a reason for hiding this comment

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

It may be set to MaxAnalysisRecursionDepth - 1 by the caller to avoid recursion.

}

bool llvm::MaskedValueIsZero(const Value *V, const APInt &Mask,
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5412,7 +5412,7 @@ Instruction *InstCombinerImpl::foldICmpBinOp(ICmpInst &I,
if (ICmpInst::isEquality(Pred)) {
// If X != Y, fold (X *nw Z) eq/ne (Y *nw Z) -> Z eq/ne 0
if (((Op0HasNSW && Op1HasNSW) || (Op0HasNUW && Op1HasNUW)) &&
isKnownNonEqual(X, Y, DL, &AC, &I, &DT))
isKnownNonEqual(X, Y, SQ))
return new ICmpInst(Pred, Z, Constant::getNullValue(Z->getType()));

KnownBits ZKnown = computeKnownBits(Z, 0, &I);
Expand Down
6 changes: 1 addition & 5 deletions llvm/test/Transforms/InstCombine/known-bits.ll
Original file line number Diff line number Diff line change
Expand Up @@ -1609,17 +1609,13 @@ if.else:
ret i16 0
}

; TODO: %cmp always evaluates to false

define i1 @test_simplify_icmp2(double %x) {
; CHECK-LABEL: @test_simplify_icmp2(
; CHECK-NEXT: [[ABS:%.*]] = tail call double @llvm.fabs.f64(double [[X:%.*]])
; CHECK-NEXT: [[COND:%.*]] = fcmp oeq double [[ABS]], 0x7FF0000000000000
; CHECK-NEXT: br i1 [[COND]], label [[IF_THEN:%.*]], label [[IF_ELSE:%.*]]
; CHECK: if.then:
; CHECK-NEXT: [[CAST:%.*]] = bitcast double [[X]] to i64
; CHECK-NEXT: [[CMP:%.*]] = icmp eq i64 [[CAST]], 3458764513820540928
; CHECK-NEXT: ret i1 [[CMP]]
; CHECK-NEXT: ret i1 false
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand why this change isn't NFC

Copy link
Member Author

Choose a reason for hiding this comment

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

With DC we will have a more precise KnownBits:

if (V1->getType()->isIntOrIntVectorTy()) {
// Are any known bits in V1 contradictory to known bits in V2? If V1
// has a known zero where V2 has a known one, they must not be equal.
KnownBits Known1 = computeKnownBits(V1, DemandedElts, Depth, Q);
if (!Known1.isUnknown()) {
KnownBits Known2 = computeKnownBits(V2, DemandedElts, Depth, Q);
if (Known1.Zero.intersects(Known2.One) ||
Known2.Zero.intersects(Known1.One))
return true;
}
}

; CHECK: if.else:
; CHECK-NEXT: ret i1 false
;
Expand Down