-
Notifications
You must be signed in to change notification settings - Fork 15k
[SCEV] Fix NSW flag propagation in getGEPExpr, getMulExpr, and getAddExpr #155145
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
Changes from all commits
a2aec67
696f52c
ce3569e
0aa25ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2951,25 +2951,52 @@ const SCEV *ScalarEvolution::getAddExpr(SmallVectorImpl<const SCEV *> &Ops, | |
| if (AddRecLoop == cast<SCEVAddRecExpr>(Ops[OtherIdx])->getLoop()) { | ||
| // Other + {A,+,B}<L> + {C,+,D}<L> --> Other + {A+C,+,B+D}<L> | ||
| SmallVector<const SCEV *, 4> AddRecOps(AddRec->operands()); | ||
|
|
||
| // Track flags: start with the flags from the first AddRec. | ||
| bool AllHaveNSW = AddRec->hasNoSignedWrap(); | ||
| bool AllHaveNUW = AddRec->hasNoUnsignedWrap(); | ||
|
|
||
| for (; OtherIdx != Ops.size() && isa<SCEVAddRecExpr>(Ops[OtherIdx]); | ||
| ++OtherIdx) { | ||
| const auto *OtherAddRec = cast<SCEVAddRecExpr>(Ops[OtherIdx]); | ||
| if (OtherAddRec->getLoop() == AddRecLoop) { | ||
| // Update flags based on this AddRec | ||
| if (!OtherAddRec->hasNoSignedWrap()) | ||
| AllHaveNSW = false; | ||
| if (!OtherAddRec->hasNoUnsignedWrap()) | ||
| AllHaveNUW = false; | ||
| for (unsigned i = 0, e = OtherAddRec->getNumOperands(); | ||
| i != e; ++i) { | ||
| if (i >= AddRecOps.size()) { | ||
| append_range(AddRecOps, OtherAddRec->operands().drop_front(i)); | ||
| break; | ||
| } | ||
| // Preserve no-wrap flags when combining AddRec operands. | ||
| SCEV::NoWrapFlags CombineFlags = SCEV::FlagAnyWrap; | ||
| if (auto *AR1 = dyn_cast<SCEVAddRecExpr>(AddRecOps[i])) | ||
| if (auto *AR2 = | ||
| dyn_cast<SCEVAddRecExpr>(OtherAddRec->getOperand(i))) { | ||
| if (AR1->hasNoSignedWrap() && AR2->hasNoSignedWrap()) | ||
| CombineFlags = setFlags(CombineFlags, SCEV::FlagNSW); | ||
| if (AR1->hasNoUnsignedWrap() && AR2->hasNoUnsignedWrap()) | ||
| CombineFlags = setFlags(CombineFlags, SCEV::FlagNUW); | ||
| } | ||
| SmallVector<const SCEV *, 2> TwoOps = { | ||
| AddRecOps[i], OtherAddRec->getOperand(i)}; | ||
| AddRecOps[i] = getAddExpr(TwoOps, SCEV::FlagAnyWrap, Depth + 1); | ||
| AddRecOps[i] = getAddExpr(TwoOps, CombineFlags, Depth + 1); | ||
| } | ||
| Ops.erase(Ops.begin() + OtherIdx); --OtherIdx; | ||
| } | ||
| } | ||
| // Step size has changed, so we cannot guarantee no self-wraparound. | ||
| Ops[Idx] = getAddRecExpr(AddRecOps, AddRecLoop, SCEV::FlagAnyWrap); | ||
| // However, preserve NSW/NUW flags if all combined AddRecs had them. | ||
| SCEV::NoWrapFlags FinalFlags = SCEV::FlagAnyWrap; | ||
| if (AllHaveNSW) | ||
| FinalFlags = setFlags(FinalFlags, SCEV::FlagNSW); | ||
| if (AllHaveNUW) | ||
| FinalFlags = setFlags(FinalFlags, SCEV::FlagNUW); | ||
|
|
||
| Ops[Idx] = getAddRecExpr(AddRecOps, AddRecLoop, FinalFlags); | ||
| return getAddExpr(Ops, SCEV::FlagAnyWrap, Depth + 1); | ||
| } | ||
| } | ||
|
|
@@ -3252,7 +3279,7 @@ const SCEV *ScalarEvolution::getMulExpr(SmallVectorImpl<const SCEV *> &Ops, | |
| // NLI * LI * {Start,+,Step} --> NLI * {LI*Start,+,LI*Step} | ||
| SmallVector<const SCEV *, 4> NewOps; | ||
| NewOps.reserve(AddRec->getNumOperands()); | ||
| const SCEV *Scale = getMulExpr(LIOps, SCEV::FlagAnyWrap, Depth + 1); | ||
| const SCEV *Scale = getMulExpr(LIOps, OrigFlags, Depth + 1); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this part of the change is correct. |
||
|
|
||
| // If both the mul and addrec are nuw, we can preserve nuw. | ||
| // If both the mul and addrec are nsw, we can only preserve nsw if either | ||
|
|
@@ -3261,6 +3288,15 @@ const SCEV *ScalarEvolution::getMulExpr(SmallVectorImpl<const SCEV *> &Ops, | |
| SCEV::NoWrapFlags Flags = | ||
| AddRec->getNoWrapFlags(ComputeFlags({Scale, AddRec})); | ||
|
|
||
| // Preserve flags for positive constant Scale. | ||
| if (auto *SC = dyn_cast<SCEVConstant>(Scale)) | ||
| if (SC->getAPInt().isStrictlyPositive()) { | ||
| if (hasFlags(OrigFlags, SCEV::FlagNSW)) | ||
| Flags = setFlags(Flags, SCEV::FlagNSW); | ||
| if (hasFlags(OrigFlags, SCEV::FlagNUW)) | ||
| Flags = setFlags(Flags, SCEV::FlagNUW); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, the claim you're making here is that But even for non-trivial cases, let's take |
||
|
|
||
| for (unsigned i = 0, e = AddRec->getNumOperands(); i != e; ++i) { | ||
| NewOps.push_back(getMulExpr(Scale, AddRec->getOperand(i), | ||
| SCEV::FlagAnyWrap, Depth + 1)); | ||
|
|
@@ -3270,7 +3306,9 @@ const SCEV *ScalarEvolution::getMulExpr(SmallVectorImpl<const SCEV *> &Ops, | |
| Instruction::Mul, getSignedRange(Scale), | ||
| OverflowingBinaryOperator::NoSignedWrap); | ||
| if (!NSWRegion.contains(getSignedRange(AddRec->getOperand(i)))) | ||
| Flags = clearFlags(Flags, SCEV::FlagNSW); | ||
| if (!hasFlags(OrigFlags, SCEV::FlagNSW)) | ||
| Flags = clearFlags(Flags, SCEV::FlagNSW); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code doesn't make sense anymore: It removes nsw only if nsw is already not set. |
||
| // Keep NSW flag if it was in OrigFlags. | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -3760,6 +3798,31 @@ ScalarEvolution::getGEPExpr(GEPOperator *GEP, | |
| if (NW.hasNoUnsignedWrap()) | ||
| OffsetWrap = setFlags(OffsetWrap, SCEV::FlagNUW); | ||
|
|
||
| // Inherit flags from index expressions when GEP has no explicit flags. | ||
| if (OffsetWrap == SCEV::FlagAnyWrap) { | ||
| // Check if all index expressions have compatible no-wrap flags | ||
| bool AllHaveNSW = true, AllHaveNUW = true; | ||
| for (const SCEV *IndexExpr : IndexExprs) { | ||
| if (auto *AR = dyn_cast<SCEVAddRecExpr>(IndexExpr)) { | ||
| if (!AR->hasNoSignedWrap()) | ||
| AllHaveNSW = false; | ||
| if (!AR->hasNoUnsignedWrap()) | ||
| AllHaveNUW = false; | ||
| } else { | ||
| // Be conservative for non-AddRec expressions. | ||
| AllHaveNSW = false; | ||
| AllHaveNUW = false; | ||
| break; | ||
| } | ||
| } | ||
| // Inherit NSW if all have NSW. | ||
| if (AllHaveNSW) | ||
| OffsetWrap = setFlags(OffsetWrap, SCEV::FlagNSW); | ||
| // Inherit NUW if all have NUW. | ||
| if (AllHaveNUW) | ||
| OffsetWrap = setFlags(OffsetWrap, SCEV::FlagNUW); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get what you're doing here. E.g. if you have |
||
|
|
||
| Type *CurTy = GEP->getType(); | ||
| bool FirstIter = true; | ||
| SmallVector<const SCEV *, 4> Offsets; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have
{0,+,1}<nuw> + {0,+,1}<nuw>, then there is a priori no reason to believe that{0,+,2}is also<nuw>, which I think is the claim you're trying to make here?