[PATCH] D141850: [SCEV] Preserve divisibility and min/max information in applyLoopGuards
Max Kazantsev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 24 01:54:05 PST 2023
mkazantsev requested changes to this revision.
mkazantsev added a comment.
This revision now requires changes to proceed.
Some nits, and potentially a bug in formula.
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15018
+ const SCEV *&LHS, const SCEV *&RHS) {
+ if (auto MinMax = dyn_cast<SCEVMinMaxExpr>(Expr)) {
+ SCTy = MinMax->getSCEVType();
----------------
caojoshua wrote:
> SequentialMinMax SCEVs can be applied here as well.
I'd prefer it to be a separate patch, if it's legal at all. Need to check carefully how poison flows in these formulae.
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15021
+ LHS = MinMax->getOperand(0);
+ RHS = MinMax->getOperand(1);
+ if (isa<SCEVConstant>(LHS))
----------------
MinMax can have more than 2 operands. Check that it is exactly 2 of them?
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15030
+ // Return a new SCEV that modifies \p Expr to the closest number divides by
+ // \p Divisor and greater than Expr.
+ auto GetNextSCEVDividesByDivisor = [&](const SCEV *Expr,
----------------
greater or equal
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15033
+ const SCEV *Divisor) {
+ if (!isKnownNonNegative(Expr) || !isKnownPositive(Divisor))
+ return Expr;
----------------
If I'm reading this correctly, this is a guarantee of no-overflow for computations you are doing. Maybe add this comment explicitly?
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15038
+ // return the SCEV: Expr + Divisor - Expr % Divisor
+ return getAddExpr(getMinusSCEV(Divisor, Rem), Expr);
+ }
----------------
caojoshua wrote:
> Have not thought about it too deeply yet, but I'm concerned that we may need to take a look at when this is legal given Expr's NoWrapFlags. Same for the equivalent getMinusSCEV below
This computation is inconsistent. If `Rem` is constant `0`, you'll return `Expr`. But if `Rem` is effectively zero, but not a constant (e.g. some complex expression which is always zero), you return `Expr + Divisor`. Bug?
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15044
+ // Return a new SCEV that modifies \p Expr to the closest number divides by
+ // \p Divisor and less than Expr.
+ auto GetPreviousSCEVDividesByDivisor = [&](const SCEV *Expr,
----------------
less or equal
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15052
+ // return the SCEV: Expr - Expr % Divisor
+ return getMinusSCEV(Expr, Rem);
+ }
----------------
I think you can safely drop check for `Rem->isZero()` here as it will be trivially simplified away in `getMinusSCEV`
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15060
+ // Divisor.
+ std::function<const SCEV *(const SCEV *, const SCEV *)>
+ ApplyDivisibiltyOnMinMaxExpr = [&](const SCEV *MinMaxExpr,
----------------
auto
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15123
+ // DividesBy.
+ std::function<bool(const SCEV *, const SCEV *&)> HasDivisibiltyInfo =
+ [&](const SCEV *Expr, const SCEV *&DividesBy) {
----------------
auto
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15137
+ }
+ if (isa<SCEVMinMaxExpr>(Expr)) {
+ auto MinMax = cast<SCEVMinMaxExpr>(Expr);
----------------
`if (auto *MinMax = dyn_cast<SCEVMinMaxExpr>(Expr))`
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15146
+ // Return true if Expr known to divide by \p DividesBy.
+ std::function<bool(const SCEV *, const SCEV *&)> IsKnownToDivideBy =
+ [&](const SCEV *Expr, const SCEV *DividesBy) {
----------------
auto
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141850/new/
https://reviews.llvm.org/D141850
More information about the llvm-commits
mailing list