Skip to content

[SCEVExpander] Expand UDiv avoiding UB when in seq_min/max. #92177

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 17 commits into from
Oct 17, 2024
Merged

[SCEVExpander] Expand UDiv avoiding UB when in seq_min/max. #92177

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
bff4e5a
[LV] Don't vectorize if trip count expansion may introduce UB.
fhahn May 14, 2024
16a4b00
Merge remote-tracking branch 'origin/main' into scevexpansion-may-int…
fhahn Sep 27, 2024
32833a7
!fixup
fhahn Sep 27, 2024
b0d098c
!fixup incluede missing change
fhahn Sep 27, 2024
d6d8655
Merge remote-tracking branch 'origin/main' into scevexpansion-may-int…
fhahn Sep 28, 2024
4ace524
!fixup update test
fhahn Sep 28, 2024
70f2ef7
!fixup address latest comments, thanks!
fhahn Sep 28, 2024
fa12ea4
Merge remote-tracking branch 'origin/main' into scevexpansion-may-int…
fhahn Oct 2, 2024
5b1a789
!fixup remove unneeded function, add SCEV test.
fhahn Oct 2, 2024
c2dc23d
Merge remote-tracking branch 'origin/main' into scevexpansion-may-int…
fhahn Oct 2, 2024
1468d8b
Merge remote-tracking branch 'origin/main' into scevexpansion-may-int…
fhahn Oct 14, 2024
c12b25c
!fixup address latest comments, thanks!
fhahn Oct 15, 2024
5f31e85
Merge remote-tracking branch 'origin/main' into scevexpansion-may-int…
fhahn Oct 16, 2024
9c4b76d
!fixup add test
fhahn Oct 16, 2024
b699ddd
Merge remote-tracking branch 'origin/main' into scevexpansion-may-int…
fhahn Oct 16, 2024
5469289
Merge remote-tracking branch 'origin/main' into scevexpansion-may-int…
fhahn Oct 16, 2024
d5c8931
!fixup also generate umax if the value may be poison.
fhahn Oct 16, 2024
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
5 changes: 5 additions & 0 deletions llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,11 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
/// "expanded" form.
bool LSRMode;

/// When true, rewrite any divisors of UDiv expressions that may be 0 to
/// umax(Divisor, 1) to avoid introducing UB. If the divisor may be poison,
/// freeze it first.
bool SafeUDivMode = false;

typedef IRBuilder<InstSimplifyFolder, IRBuilderCallbackInserter> BuilderType;
BuilderType Builder;

Expand Down
13 changes: 12 additions & 1 deletion llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,14 @@ Value *SCEVExpander::visitUDivExpr(const SCEVUDivExpr *S) {
SCEV::FlagAnyWrap, /*IsSafeToHoist*/ true);
}

Value *RHS = expand(S->getRHS());
const SCEV *RHSExpr = S->getRHS();
Value *RHS = expand(RHSExpr);
if (SafeUDivMode && !SE.isKnownNonZero(RHSExpr)) {
Copy link
Collaborator

@efriedma-quic efriedma-quic Oct 15, 2024

Choose a reason for hiding this comment

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

isKnownNonZero doesn't check for poison, I think.

Copy link
Contributor Author

@fhahn fhahn Oct 15, 2024

Choose a reason for hiding this comment

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

Hm, let me see if I can come up with a case where it is known-non-zero, but may be poison

Copy link
Collaborator

@efriedma-quic efriedma-quic Oct 15, 2024

Choose a reason for hiding this comment

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

Something like umax(x, 1) should do the trick.

Copy link
Contributor Author

@fhahn fhahn Oct 16, 2024

Choose a reason for hiding this comment

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

Added and updated to separately check for !poison -> emit freeze, !known-non-zero -> emit umax

if (!ScalarEvolution::isGuaranteedNotToBePoison(RHSExpr))
RHS = Builder.CreateFreeze(RHS);
RHS = Builder.CreateIntrinsic(RHS->getType(), Intrinsic::umax,
{RHS, ConstantInt::get(RHS->getType(), 1)});
}
return InsertBinop(Instruction::UDiv, LHS, RHS, SCEV::FlagAnyWrap,
/*IsSafeToHoist*/ SE.isKnownNonZero(S->getRHS()));
}
Expand Down Expand Up @@ -1371,11 +1378,14 @@ Value *SCEVExpander::visitSignExtendExpr(const SCEVSignExtendExpr *S) {
Value *SCEVExpander::expandMinMaxExpr(const SCEVNAryExpr *S,
Intrinsic::ID IntrinID, Twine Name,
bool IsSequential) {
bool PrevSafeMode = SafeUDivMode;
SafeUDivMode |= IsSequential;
Value *LHS = expand(S->getOperand(S->getNumOperands() - 1));
Type *Ty = LHS->getType();
if (IsSequential)
LHS = Builder.CreateFreeze(LHS);
for (int i = S->getNumOperands() - 2; i >= 0; --i) {
SafeUDivMode = (IsSequential && i != 0) || PrevSafeMode;
Value *RHS = expand(S->getOperand(i));
if (IsSequential && i != 0)
RHS = Builder.CreateFreeze(RHS);
Expand All @@ -1390,6 +1400,7 @@ Value *SCEVExpander::expandMinMaxExpr(const SCEVNAryExpr *S,
}
LHS = Sel;
}
SafeUDivMode = PrevSafeMode;
return LHS;
}

Expand Down
Loading