[LVI][CVP] Treat undef like a full range #68190
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DianQK
force-pushed
the
lvi-propagate-undef
branch
from
be2a22b
to
488367e
Compare
DianQK
changed the title
getRangeFor propagates MayIncludeUndefundef to range and abs
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis ChangesFixes #68381. Full diff: https://github.com/llvm/llvm-project/pull/68190.diff 4 Files Affected:
diff --git a/llvm/include/llvm/Analysis/LazyValueInfo.h b/llvm/include/llvm/Analysis/LazyValueInfo.h
index f013a4a75d3d6ad..55f9b3c14ca673d 100644
--- a/llvm/include/llvm/Analysis/LazyValueInfo.h
+++ b/llvm/include/llvm/Analysis/LazyValueInfo.h
@@ -70,14 +70,16 @@ namespace llvm {
/// predicate.
Tristate getPredicateOnEdge(unsigned Pred, Value *V, Constant *C,
BasicBlock *FromBB, BasicBlock *ToBB,
- Instruction *CxtI = nullptr);
+ Instruction *CxtI = nullptr,
+ bool UndefinedAllowed = true);
/// Determine whether the specified value comparison with a constant is
/// known to be true or false at the specified instruction. \p Pred is a
/// CmpInst predicate. If \p UseBlockValue is true, the block value is also
/// taken into account.
Tristate getPredicateAt(unsigned Pred, Value *V, Constant *C,
- Instruction *CxtI, bool UseBlockValue);
+ Instruction *CxtI, bool UseBlockValue,
+ bool UndefinedAllowed = true);
/// Determine whether the specified value comparison is known to be true
/// or false at the specified instruction. While this takes two Value's,
diff --git a/llvm/lib/Analysis/LazyValueInfo.cpp b/llvm/lib/Analysis/LazyValueInfo.cpp
index 0892aa9d75fb417..d4da9c05ccfed90 100644
--- a/llvm/lib/Analysis/LazyValueInfo.cpp
+++ b/llvm/lib/Analysis/LazyValueInfo.cpp
@@ -411,7 +411,8 @@ class LazyValueInfoImpl {
std::optional<ValueLatticeElement> solveBlockValueSelect(SelectInst *S,
BasicBlock *BB);
std::optional<ConstantRange> getRangeFor(Value *V, Instruction *CxtI,
- BasicBlock *BB);
+ BasicBlock *BB,
+ bool &MayIncludeUndef);
std::optional<ValueLatticeElement> solveBlockValueBinaryOpImpl(
Instruction *I, BasicBlock *BB,
std::function<ConstantRange(const ConstantRange &, const ConstantRange &)>
@@ -892,11 +893,14 @@ LazyValueInfoImpl::solveBlockValueSelect(SelectInst *SI, BasicBlock *BB) {
}
std::optional<ConstantRange>
-LazyValueInfoImpl::getRangeFor(Value *V, Instruction *CxtI, BasicBlock *BB) {
+LazyValueInfoImpl::getRangeFor(Value *V, Instruction *CxtI, BasicBlock *BB,
+ bool &MayIncludeUndef) {
std::optional<ValueLatticeElement> OptVal = getBlockValue(V, BB, CxtI);
if (!OptVal)
return std::nullopt;
- return getConstantRangeOrFull(*OptVal, V->getType(), DL);
+ const auto &Val = *OptVal;
+ MayIncludeUndef = Val.isConstantRangeIncludingUndef();
+ return getConstantRangeOrFull(Val, V->getType(), DL);
}
std::optional<ValueLatticeElement>
@@ -922,10 +926,12 @@ LazyValueInfoImpl::solveBlockValueCast(CastInst *CI, BasicBlock *BB) {
return ValueLatticeElement::getOverdefined();
}
+ bool MayIncludeUndef = false;
// Figure out the range of the LHS. If that fails, we still apply the
// transfer rule on the full set since we may be able to locally infer
// interesting facts.
- std::optional<ConstantRange> LHSRes = getRangeFor(CI->getOperand(0), CI, BB);
+ std::optional<ConstantRange> LHSRes =
+ getRangeFor(CI->getOperand(0), CI, BB, MayIncludeUndef);
if (!LHSRes)
// More work to do before applying this transfer rule.
return std::nullopt;
@@ -936,8 +942,8 @@ LazyValueInfoImpl::solveBlockValueCast(CastInst *CI, BasicBlock *BB) {
// NOTE: We're currently limited by the set of operations that ConstantRange
// can evaluate symbolically. Enhancing that set will allows us to analyze
// more definitions.
- return ValueLatticeElement::getRange(LHSRange.castOp(CI->getOpcode(),
- ResultBitWidth));
+ return ValueLatticeElement::getRange(
+ LHSRange.castOp(CI->getOpcode(), ResultBitWidth), MayIncludeUndef);
}
std::optional<ValueLatticeElement>
@@ -949,15 +955,20 @@ LazyValueInfoImpl::solveBlockValueBinaryOpImpl(
// conservative range, but apply the transfer rule anyways. This
// lets us pick up facts from expressions like "and i32 (call i32
// @foo()), 32"
- std::optional<ConstantRange> LHSRes = getRangeFor(I->getOperand(0), I, BB);
- std::optional<ConstantRange> RHSRes = getRangeFor(I->getOperand(1), I, BB);
+ bool LMayIncludeUndef = false;
+ bool RMayIncludeUndef = false;
+ std::optional<ConstantRange> LHSRes =
+ getRangeFor(I->getOperand(0), I, BB, LMayIncludeUndef);
+ std::optional<ConstantRange> RHSRes =
+ getRangeFor(I->getOperand(1), I, BB, RMayIncludeUndef);
if (!LHSRes || !RHSRes)
// More work to do before applying this transfer rule.
return std::nullopt;
const ConstantRange &LHSRange = *LHSRes;
const ConstantRange &RHSRange = *RHSRes;
- return ValueLatticeElement::getRange(OpFn(LHSRange, RHSRange));
+ return ValueLatticeElement::getRange(OpFn(LHSRange, RHSRange),
+ LMayIncludeUndef || RMayIncludeUndef);
}
std::optional<ValueLatticeElement>
@@ -1002,16 +1013,21 @@ LazyValueInfoImpl::solveBlockValueIntrinsic(IntrinsicInst *II, BasicBlock *BB) {
return MetadataVal;
}
+ bool ArgsMayIncludeUndef = false;
SmallVector<ConstantRange, 2> OpRanges;
for (Value *Op : II->args()) {
- std::optional<ConstantRange> Range = getRangeFor(Op, II, BB);
+ bool MayIncludeUndef = false;
+ std::optional<ConstantRange> Range =
+ getRangeFor(Op, II, BB, MayIncludeUndef);
if (!Range)
return std::nullopt;
+ ArgsMayIncludeUndef |= MayIncludeUndef;
OpRanges.push_back(*Range);
}
- return intersect(ValueLatticeElement::getRange(ConstantRange::intrinsic(
- II->getIntrinsicID(), OpRanges)),
+ return intersect(ValueLatticeElement::getRange(
+ ConstantRange::intrinsic(II->getIntrinsicID(), OpRanges),
+ ArgsMayIncludeUndef),
MetadataVal);
}
@@ -1750,7 +1766,8 @@ ConstantRange LazyValueInfo::getConstantRangeOnEdge(Value *V,
static LazyValueInfo::Tristate
getPredicateResult(unsigned Pred, Constant *C, const ValueLatticeElement &Val,
- const DataLayout &DL, TargetLibraryInfo *TLI) {
+ const DataLayout &DL, TargetLibraryInfo *TLI,
+ bool UndefinedAllowed = true) {
// If we know the value is a constant, evaluate the conditional.
Constant *Res = nullptr;
if (Val.isConstant()) {
@@ -1761,6 +1778,8 @@ getPredicateResult(unsigned Pred, Constant *C, const ValueLatticeElement &Val,
}
if (Val.isConstantRange()) {
+ if (!UndefinedAllowed && Val.isConstantRangeIncludingUndef())
+ return LazyValueInfo::Unknown;
ConstantInt *CI = dyn_cast<ConstantInt>(C);
if (!CI) return LazyValueInfo::Unknown;
@@ -1818,17 +1837,20 @@ getPredicateResult(unsigned Pred, Constant *C, const ValueLatticeElement &Val,
LazyValueInfo::Tristate
LazyValueInfo::getPredicateOnEdge(unsigned Pred, Value *V, Constant *C,
BasicBlock *FromBB, BasicBlock *ToBB,
- Instruction *CxtI) {
+ Instruction *CxtI, bool UndefinedAllowed) {
Module *M = FromBB->getModule();
ValueLatticeElement Result =
getOrCreateImpl(M).getValueOnEdge(V, FromBB, ToBB, CxtI);
- return getPredicateResult(Pred, C, Result, M->getDataLayout(), TLI);
+ return getPredicateResult(Pred, C, Result, M->getDataLayout(), TLI,
+ UndefinedAllowed);
}
-LazyValueInfo::Tristate
-LazyValueInfo::getPredicateAt(unsigned Pred, Value *V, Constant *C,
- Instruction *CxtI, bool UseBlockValue) {
+LazyValueInfo::Tristate LazyValueInfo::getPredicateAt(unsigned Pred, Value *V,
+ Constant *C,
+ Instruction *CxtI,
+ bool UseBlockValue,
+ bool UndefinedAllowed) {
// Is or is not NonNull are common predicates being queried. If
// isKnownNonZero can tell us the result of the predicate, we can
// return it quickly. But this is only a fastpath, and falling
@@ -1847,7 +1869,7 @@ LazyValueInfo::getPredicateAt(unsigned Pred, Value *V, Constant *C,
ValueLatticeElement Result =
UseBlockValue ? Impl.getValueInBlock(V, CxtI->getParent(), CxtI)
: Impl.getValueAt(V, CxtI);
- Tristate Ret = getPredicateResult(Pred, C, Result, DL, TLI);
+ Tristate Ret = getPredicateResult(Pred, C, Result, DL, TLI, UndefinedAllowed);
if (Ret != Unknown)
return Ret;
@@ -1892,8 +1914,8 @@ LazyValueInfo::getPredicateAt(unsigned Pred, Value *V, Constant *C,
Value *Incoming = PHI->getIncomingValue(i);
BasicBlock *PredBB = PHI->getIncomingBlock(i);
// Note that PredBB may be BB itself.
- Tristate Result =
- getPredicateOnEdge(Pred, Incoming, C, PredBB, BB, CxtI);
+ Tristate Result = getPredicateOnEdge(Pred, Incoming, C, PredBB, BB,
+ CxtI, UndefinedAllowed);
// Keep going as long as we've seen a consistent known result for
// all inputs.
@@ -1914,11 +1936,13 @@ LazyValueInfo::getPredicateAt(unsigned Pred, Value *V, Constant *C,
// For predecessor edge, determine if the comparison is true or false
// on that edge. If they're all true or all false, we can conclude
// the value of the comparison in this block.
- Tristate Baseline = getPredicateOnEdge(Pred, V, C, *PI, BB, CxtI);
+ Tristate Baseline =
+ getPredicateOnEdge(Pred, V, C, *PI, BB, CxtI, UndefinedAllowed);
if (Baseline != Unknown) {
// Check that all remaining incoming values match the first one.
while (++PI != PE) {
- Tristate Ret = getPredicateOnEdge(Pred, V, C, *PI, BB, CxtI);
+ Tristate Ret =
+ getPredicateOnEdge(Pred, V, C, *PI, BB, CxtI, UndefinedAllowed);
if (Ret != Baseline)
break;
}
diff --git a/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp b/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
index 48b27a1ea0a2984..b5d34638456af41 100644
--- a/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
+++ b/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
@@ -479,7 +479,7 @@ static bool processAbsIntrinsic(IntrinsicInst *II, LazyValueInfo *LVI) {
// Is X in [0, IntMin]? NOTE: INT_MIN is fine!
Result = LVI->getPredicateAt(CmpInst::Predicate::ICMP_ULE, X, IntMin, II,
- /*UseBlockValue=*/true);
+ /*UseBlockValue=*/true, IsIntMinPoison);
if (Result == LazyValueInfo::True) {
++NumAbs;
II->replaceAllUsesWith(X);
@@ -490,7 +490,7 @@ static bool processAbsIntrinsic(IntrinsicInst *II, LazyValueInfo *LVI) {
// Is X in [IntMin, 0]? NOTE: INT_MIN is fine!
Constant *Zero = ConstantInt::getNullValue(Ty);
Result = LVI->getPredicateAt(CmpInst::Predicate::ICMP_SLE, X, Zero, II,
- /*UseBlockValue=*/true);
+ /*UseBlockValue=*/true, IsIntMinPoison);
assert(Result != LazyValueInfo::False && "Should have been handled already.");
if (Result == LazyValueInfo::Unknown) {
@@ -499,7 +499,7 @@ static bool processAbsIntrinsic(IntrinsicInst *II, LazyValueInfo *LVI) {
if (!IsIntMinPoison) {
// Can we at least tell that the argument is never INT_MIN?
Result = LVI->getPredicateAt(CmpInst::Predicate::ICMP_NE, X, IntMin, II,
- /*UseBlockValue=*/true);
+ /*UseBlockValue=*/true, IsIntMinPoison);
if (Result == LazyValueInfo::True) {
++NumNSW;
++NumSubNSW;
diff --git a/llvm/test/Transforms/CorrelatedValuePropagation/merge-range-and-undef-2.ll b/llvm/test/Transforms/CorrelatedValuePropagation/merge-range-and-undef-2.ll
new file mode 100644
index 000000000000000..4188f5e732d640d
--- /dev/null
+++ b/llvm/test/Transforms/CorrelatedValuePropagation/merge-range-and-undef-2.ll
@@ -0,0 +1,109 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 3
+; RUN: opt -S -passes=correlated-propagation %s | FileCheck %s
+
+; Test case for PR68381.
+
+; Don't delete the `and` instruction.
+
+define i32 @constant_range_and_undef_and(i1 %c0, i1 %c1, i8 %v1, i8 %v2) {
+; CHECK-LABEL: define i32 @constant_range_and_undef_and(
+; CHECK-SAME: i1 [[C0:%.*]], i1 [[C1:%.*]], i8 [[V1:%.*]], i8 [[V2:%.*]]) {
+; CHECK-NEXT: start:
+; CHECK-NEXT: br i1 [[C0]], label [[BB0:%.*]], label [[BB1:%.*]]
+; CHECK: bb0:
+; CHECK-NEXT: [[V1_I32:%.*]] = zext i8 [[V1]] to i32
+; CHECK-NEXT: br label [[BB1]]
+; CHECK: bb1:
+; CHECK-NEXT: [[X:%.*]] = phi i32 [ [[V1_I32]], [[BB0]] ], [ undef, [[START:%.*]] ]
+; CHECK-NEXT: br i1 [[C1]], label [[BB0]], label [[BB2:%.*]]
+; CHECK: bb2:
+; CHECK-NEXT: [[V2_I32:%.*]] = zext i8 [[V2]] to i32
+; CHECK-NEXT: [[Y:%.*]] = or i32 [[X]], [[V2_I32]]
+; CHECK-NEXT: [[Z:%.*]] = and i32 [[Y]], 255
+; CHECK-NEXT: ret i32 [[Z]]
+;
+start:
+ br i1 %c0, label %bb0, label %bb1
+
+bb0:
+ %v1_i32 = zext i8 %v1 to i32
+ br label %bb1
+
+bb1:
+ %x = phi i32 [ %v1_i32, %bb0 ], [ undef, %start ]
+ br i1 %c1, label %bb0, label %bb2
+
+bb2:
+ %v2_i32 = zext i8 %v2 to i32
+ %y = or i32 %x, %v2_i32
+ %z = and i32 %y, 255
+ ret i32 %z
+}
+
+; Don't delete the `abs` intrinsic function.
+
+define i32 @constant_range_and_undef_abs_false(i1 %c0, i1 %c1, i8 %v1) {
+; CHECK-LABEL: define i32 @constant_range_and_undef_abs_false(
+; CHECK-SAME: i1 [[C0:%.*]], i1 [[C1:%.*]], i8 [[V1:%.*]]) {
+; CHECK-NEXT: start:
+; CHECK-NEXT: br i1 [[C0]], label [[BB0:%.*]], label [[BB1:%.*]]
+; CHECK: bb0:
+; CHECK-NEXT: [[V1_I32:%.*]] = zext i8 [[V1]] to i32
+; CHECK-NEXT: br label [[BB1]]
+; CHECK: bb1:
+; CHECK-NEXT: [[X:%.*]] = phi i32 [ [[V1_I32]], [[BB0]] ], [ undef, [[START:%.*]] ]
+; CHECK-NEXT: br i1 [[C1]], label [[BB0]], label [[BB2:%.*]]
+; CHECK: bb2:
+; CHECK-NEXT: [[Z:%.*]] = call i32 @llvm.abs.i32(i32 [[X]], i1 false)
+; CHECK-NEXT: ret i32 [[Z]]
+;
+start:
+ br i1 %c0, label %bb0, label %bb1
+
+bb0:
+ %v1_i32 = zext i8 %v1 to i32
+ br label %bb1
+
+bb1:
+ %x = phi i32 [ %v1_i32, %bb0 ], [ undef, %start ]
+ br i1 %c1, label %bb0, label %bb2
+
+bb2:
+ %z = call i32 @llvm.abs.i32(i32 %x, i1 false)
+ ret i32 %z
+}
+
+; We can delete the `abs` intrinsic function.
+
+define i32 @constant_range_and_undef_true(i1 %c0, i1 %c1, i8 %v1) {
+; CHECK-LABEL: define i32 @constant_range_and_undef_true(
+; CHECK-SAME: i1 [[C0:%.*]], i1 [[C1:%.*]], i8 [[V1:%.*]]) {
+; CHECK-NEXT: start:
+; CHECK-NEXT: br i1 [[C0]], label [[BB0:%.*]], label [[BB1:%.*]]
+; CHECK: bb0:
+; CHECK-NEXT: [[V1_I32:%.*]] = zext i8 [[V1]] to i32
+; CHECK-NEXT: br label [[BB1]]
+; CHECK: bb1:
+; CHECK-NEXT: [[X:%.*]] = phi i32 [ [[V1_I32]], [[BB0]] ], [ undef, [[START:%.*]] ]
+; CHECK-NEXT: br i1 [[C1]], label [[BB0]], label [[BB2:%.*]]
+; CHECK: bb2:
+; CHECK-NEXT: ret i32 [[X]]
+;
+start:
+ br i1 %c0, label %bb0, label %bb1
+
+bb0:
+ %v1_i32 = zext i8 %v1 to i32
+ br label %bb1
+
+bb1:
+ %x = phi i32 [ %v1_i32, %bb0 ], [ undef, %start ]
+ br i1 %c1, label %bb0, label %bb2
+
+bb2:
+ %z = call i32 @llvm.abs.i32(i32 %x, i1 true)
+ ret i32 %z
+}
+
+declare i32 @llvm.abs.i32(i32, i1)
+
|
|
Gently ping. @nikic |
nikic
requested changes
Oct 9, 2023
There was a problem hiding this comment.
I don't think this is the right approach to fixing this. The core fix here should be along these lines:
diff --git a/llvm/lib/Analysis/LazyValueInfo.cpp b/llvm/lib/Analysis/LazyValueInfo.cpp
index 0892aa9d75fb..789a02ed329c 100644
--- a/llvm/lib/Analysis/LazyValueInfo.cpp
+++ b/llvm/lib/Analysis/LazyValueInfo.cpp
@@ -801,7 +801,7 @@ void LazyValueInfoImpl::intersectAssumeOrGuardBlockValueConstantRange(
static ConstantRange getConstantRangeOrFull(const ValueLatticeElement &Val,
Type *Ty, const DataLayout &DL) {
- if (Val.isConstantRange())
+ if (Val.isConstantRange(/*UndefAllowed*/ false))
return Val.getConstantRange();
return ConstantRange::getFull(DL.getTypeSizeInBits(Ty));
}That is, when converting to ConstantRange, we should treat undef like a full range.
The way you are propagating undef here is basically as if it were poison, but this doesn't match its semantics. zext CR|undef does not result in CR.zext()|undef, because the top bits are guaranteed to be zero even if the input is undef.
I think this approach will result in a simpler and more robust fix.
DianQK
force-pushed
the
lvi-propagate-undef
branch
from
488367e
to
d0cd71c
Compare
Thanks for pointing that out. Yes! This is the better approach. For a better review and commit history, I will submit a separate PR for |
DianQK
changed the title
undef to range and abs
|
Can this PR be closed then? |
This PR should be merged. I made the changes in this PR as suggested by @nikic. |
|
Ah never mind. I thought it was targeted for a backport to the release, but I just mixed up my notifications. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #68381.