[VPlan] Expand sequential/regular UMin SCEVs in VPSCEVExpander. - #209786
Open
fhahn wants to merge 1 commit into
Open
[VPlan] Expand sequential/regular UMin SCEVs in VPSCEVExpander.#209786fhahn wants to merge 1 commit into
fhahn wants to merge 1 commit into
Conversation
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-vectorizers Author: Florian Hahn (fhahn) ChangesAdd support for expanding SequentialUMinExpr SCEV expressions in VPSCEVExpander. For regular UMin expressions, the expansion unconditionally expands & executes all operands, while the semantics of sequential UMin only require the first operand to be evaluated unconditionally. For sequential UMin expressions, we need to make sure potentially UB/poison generating operands must be accounted for. Matching IR SCEV expander, make sure that divisors of UDiv are poison-free and non-zero inside sequential UMin. Similarly, freeze all operands other than the first, to avoid poison from propagating. Full diff: https://github.com/llvm/llvm-project/pull/209786.diff 3 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp b/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
index 03e1ead89c169..d3aa44f9eac5c 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
@@ -812,9 +812,23 @@ VPValue *VPSCEVExpander::tryToExpand(const SCEV *S) {
VPValue *LHS = tryToExpand(UDiv->getLHS());
if (!LHS)
return nullptr;
- VPValue *RHS = tryToExpand(UDiv->getRHS());
+ const SCEV *RHSExpr = UDiv->getRHS();
+ VPValue *RHS = tryToExpand(RHSExpr);
if (!RHS)
return nullptr;
+ if (SafeUDivMode) {
+ // Make sure the UDiv's divisor is guaranteed to not be zero/poison, to
+ // avoid UB.
+ Type *RHSTy = RHSExpr->getType();
+ bool GuaranteedNotPoison =
+ ScalarEvolution::isGuaranteedNotToBePoison(RHSExpr);
+ if (!GuaranteedNotPoison)
+ RHS = Builder.createScalarFreeze(RHS, RHSTy, DL);
+ if (!SE.isKnownNonZero(RHSExpr) || !GuaranteedNotPoison)
+ RHS = Builder.createScalarIntrinsic(
+ Intrinsic::umax, {RHS, Builder.getPlan().getConstantInt(RHSTy, 1)},
+ RHSTy, DL);
+ }
return Builder.createNaryOp(Instruction::UDiv, {LHS, RHS},
VPIRFlags::getDefaultFlags(Instruction::UDiv),
DL);
@@ -853,8 +867,9 @@ VPValue *VPSCEVExpander::tryToExpand(const SCEV *S) {
case scUMaxExpr:
case scSMaxExpr:
case scUMinExpr:
- case scSMinExpr: {
- auto *MinMax = cast<SCEVMinMaxExpr>(S);
+ case scSMinExpr:
+ case scSequentialUMinExpr: {
+ auto *MinMax = cast<SCEVNAryExpr>(S);
Intrinsic::ID IntrinsicID;
switch (S->getSCEVType()) {
case scUMaxExpr:
@@ -864,6 +879,7 @@ VPValue *VPSCEVExpander::tryToExpand(const SCEV *S) {
IntrinsicID = Intrinsic::smax;
break;
case scUMinExpr:
+ case scSequentialUMinExpr:
IntrinsicID = Intrinsic::umin;
break;
case scSMinExpr:
@@ -873,19 +889,26 @@ VPValue *VPSCEVExpander::tryToExpand(const SCEV *S) {
llvm_unreachable("Unexpected min/max SCEV type");
}
// Chain operands in reverse order matching SCEVExpander's expansion of
- // min/max expressions.
- SmallVector<VPValue *, 2> Ops;
- for (const SCEVUse &Op : reverse(MinMax->operands())) {
- VPValue *OpV = tryToExpand(Op);
- if (!OpV)
+ // min/max expressions. In SafeUDivMode freeze expansion results of operands
+ // other than the first for sequential UMins, to avoid short-circuiting
+ // divide-by-0/poison.
+ bool IsSequential = S->getSCEVType() == scSequentialUMinExpr;
+ Type *ResultTy = MinMax->getType();
+ bool PrevSafeMode = SafeUDivMode;
+ VPValue *Result = nullptr;
+ for (const auto &[I, SCEVOp] : enumerate(reverse(MinMax->operands()))) {
+ bool MayShortCircuit = IsSequential && I != MinMax->getNumOperands() - 1;
+ SafeUDivMode = MayShortCircuit || PrevSafeMode;
+ VPValue *Op = tryToExpand(SCEVOp);
+ SafeUDivMode = PrevSafeMode;
+ if (!Op)
return nullptr;
- Ops.push_back(OpV);
+ if (MayShortCircuit)
+ Op = Builder.createScalarFreeze(Op, ResultTy, DL);
+ Result = Result ? Builder.createScalarIntrinsic(IntrinsicID, {Result, Op},
+ ResultTy, DL)
+ : Op;
}
- Type *ResultTy = MinMax->getType();
- VPValue *Result = Ops.front();
- for (VPValue *Op : drop_begin(Ops))
- Result = Builder.createScalarIntrinsic(IntrinsicID, {Result, Op},
- ResultTy, DL);
return Result;
}
default:
diff --git a/llvm/lib/Transforms/Vectorize/VPlanUtils.h b/llvm/lib/Transforms/Vectorize/VPlanUtils.h
index 2980b704ec8da..497ee5c66c4cd 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanUtils.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanUtils.h
@@ -177,6 +177,10 @@ class VPSCEVExpander {
ScalarEvolution &SE;
DebugLoc DL;
+ /// When true, nested SCEVUDivExprs are expanded so that they cannot divide by
+ /// zero, matching SCEVExpander's SafeUDivMode.
+ bool SafeUDivMode = false;
+
/// Try to find a loop-invariant IR value in the plan's entry block whose
/// SCEV matches \p S. Returns the corresponding live-in VPValue, or nullptr
/// if none is found.
diff --git a/llvm/test/Transforms/LoopVectorize/trip-count-expansion-may-introduce-ub.ll b/llvm/test/Transforms/LoopVectorize/trip-count-expansion-may-introduce-ub.ll
index c02847c55f23b..0cbb2aac3c22c 100644
--- a/llvm/test/Transforms/LoopVectorize/trip-count-expansion-may-introduce-ub.ll
+++ b/llvm/test/Transforms/LoopVectorize/trip-count-expansion-may-introduce-ub.ll
@@ -916,7 +916,7 @@ define i64 @multi_exit_4_exit_count_with_urem_by_value_in_latch(ptr %dst, i64 %N
; CHECK-NEXT: entry:
; CHECK-NEXT: [[TMP12:%.*]] = call i64 @llvm.umax.i64(i64 [[N]], i64 1)
; CHECK-NEXT: [[TMP0:%.*]] = udiv i64 42, [[TMP12]]
-; CHECK-NEXT: [[TMP1:%.*]] = mul nuw i64 [[N]], [[TMP0]]
+; CHECK-NEXT: [[TMP1:%.*]] = mul i64 [[N]], [[TMP0]]
; CHECK-NEXT: [[TMP2:%.*]] = sub i64 42, [[TMP1]]
; CHECK-NEXT: [[SMAX1:%.*]] = call i64 @llvm.smax.i64(i64 [[TMP2]], i64 0)
; CHECK-NEXT: [[TMP10:%.*]] = freeze i64 [[SMAX1]]
|
artagnon
reviewed
Jul 20, 2026
artagnon
left a comment
artagnon
left a comment
Contributor
There was a problem hiding this comment.
Requesting input from @efriedma-quic.
Add support for expanding SequentialUMinExpr SCEV expressions in VPSCEVExpander. For regular UMin expressions, the expansion unconditionally expands & executes all operands, while the semantics of sequential UMin only require the first operand to be evaluated unconditionally. For sequential UMin expressions, we need to make sure potentially UB/poison generating operands must be accounted for. Matching IR SCEV expander, make sure that divisors of UDiv are poison-free and non-zero inside sequential UMin. Similarly, freeze all operands other than the first, to avoid poison from propagating.
fhahn
force-pushed
the
vplan-expand-sequential-umin
branch
from
July 23, 2026 09:03
8332d36 to
e0d992b
Compare
artagnon
reviewed
Jul 27, 2026
Comment on lines
-901
to
+917
| VPValue *RHS = tryToExpand(UDiv->getRHS()); | ||
| const SCEV *RHSExpr = UDiv->getRHS(); | ||
| VPValue *RHS = tryToExpand(RHSExpr); | ||
| if (!RHS) | ||
| return nullptr; | ||
| if (SafeUDivMode) { | ||
| // Make sure the UDiv's divisor is guaranteed to not be zero/poison, to | ||
| // avoid UB. | ||
| Type *RHSTy = RHSExpr->getType(); | ||
| bool GuaranteedNotPoison = | ||
| ScalarEvolution::isGuaranteedNotToBePoison(RHSExpr); | ||
| if (!GuaranteedNotPoison) | ||
| RHS = Builder.createScalarFreeze(RHS, RHSTy, DL); | ||
| if (!SE.isKnownNonZero(RHSExpr) || !GuaranteedNotPoison) | ||
| RHS = Builder.createScalarIntrinsic( | ||
| Intrinsic::umax, {RHS, Builder.getPlan().getConstantInt(RHSTy, 1)}, | ||
| RHSTy, DL); | ||
| } |
Contributor
There was a problem hiding this comment.
Verified that this corresponds to:
const SCEV *RHSExpr = S->getRHS();
Value *RHS = expand(RHSExpr);
if (SafeUDivMode) {
bool GuaranteedNotPoison =
ScalarEvolution::isGuaranteedNotToBePoison(RHSExpr);
if (!GuaranteedNotPoison)
RHS = Builder.CreateFreeze(RHS);
// We need an umax if either RHSExpr is not known to be zero, or if it is
// not guaranteed to be non-poison. In the later case, the frozen poison may
// be 0.
if (!SE.isKnownNonZero(RHSExpr) || !GuaranteedNotPoison)
RHS = Builder.CreateIntrinsic(RHS->getType(), Intrinsic::umax,
{RHS, ConstantInt::get(RHS->getType(), 1)});I'm not sure why we use the RHS type, but I think we can just use S's type for clarity?
Comment on lines
+985
to
+986
| for (const auto &[I, SCEVOp] : enumerate(reverse(MinMax->operands()))) { | ||
| bool MayShortCircuit = IsSequential && I != MinMax->getNumOperands() - 1; |
Contributor
There was a problem hiding this comment.
Suggested change
| for (const auto &[I, SCEVOp] : enumerate(reverse(MinMax->operands()))) { | |
| bool MayShortCircuit = IsSequential && I != MinMax->getNumOperands() - 1; | |
| for (const auto &[I, SCEVOp] : reverse(enumerate(MinMax->operands()))) { | |
| bool MayShortCircuit = IsSequential && I != 0; |
?
| bool MayShortCircuit = IsSequential && I != MinMax->getNumOperands() - 1; | ||
| SafeUDivMode = MayShortCircuit || PrevSafeMode; | ||
| VPValue *Op = tryToExpand(SCEVOp); | ||
| SafeUDivMode = PrevSafeMode; |
| SafeUDivMode = PrevSafeMode; | ||
| if (!Op) | ||
| return nullptr; | ||
| Ops.push_back(OpV); |
Contributor
There was a problem hiding this comment.
Hm, we're not collecting ops as before, potentially creating dead instructions, but it shouldn't be a problem with our AddRec patch nearly ready?
Comment on lines
+994
to
+996
| Result = Result ? Builder.createScalarIntrinsic(IntrinsicID, {Result, Op}, | ||
| ResultTy, DL) | ||
| : Op; |
Contributor
There was a problem hiding this comment.
Hm, what happened to the Cmp/Sel thing on non-integral types? Was it some kind of optz in SCEVExp?
Contributor
There was a problem hiding this comment.
Would be good to have a little more test coverage showing the freezes at the very least?
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Add support for expanding SequentialUMinExpr SCEV expressions in VPSCEVExpander.
For regular UMin expressions, the expansion unconditionally expands & executes all operands, while the semantics of sequential UMin only require the first operand to be evaluated unconditionally.
For sequential UMin expressions, we need to make sure potentially UB/poison generating operands must be accounted for. Matching IR SCEV expander, make sure that divisors of UDiv are poison-free and non-zero inside sequential UMin. Similarly, freeze all operands other than the first, to avoid poison from propagating.