[PATCH] D141850: [SCEV] Preserve divisibility and min/max information in applyLoopGuards

[Alon Kom via Phabricator via llvm-commits]

alonkom marked 14 inline comments as done.
alonkom added a comment.

In D141850#4072675 <https://reviews.llvm.org/D141850#4072675>, @caojoshua wrote:

>   After this patch, depending on the assume processing order applyLoopGuards could create the following SCEV:
>   max(min((8 * (TC / 8)) , 96), 8)
>
> This example looks wrong. I think min/max should be switched. Should be
>
>   min(max((8 * (TC / 8)), 96), 8)
>
> Please update description.
>
> ---
>
> In terms of overall approach, I'm not sure. Feels a bit hacky to have custom logic to check that an expressions is a min/max of mul/div. I'll let others chime in here.

I think the example is correct. 
Even before this patch:
TC > 0 is translated to max (TC, 1)
TC < 99 is translated to min (TC, 98)



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15018
+                                        const SCEV *&LHS, const SCEV *&RHS) {
+      if (auto MinMax = dyn_cast<SCEVMinMaxExpr>(Expr)) {
+        SCTy = MinMax->getSCEVType();
----------------
mkazantsev wrote:
> 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.
They are not created in this function, so I prefer only handling MinMax at this point.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15018
+                                        const SCEV *&LHS, const SCEV *&RHS) {
+      if (auto MinMax = dyn_cast<SCEVMinMaxExpr>(Expr)) {
+        SCTy = MinMax->getSCEVType();
----------------
alonkom wrote:
> mkazantsev wrote:
> > 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.
> They are not created in this function, so I prefer only handling MinMax at this point.
can you explain what isn't legal here?
just checking if this is a min/max of 2 operands, when the 2nd one is constant.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15019
+      if (auto MinMax = dyn_cast<SCEVMinMaxExpr>(Expr)) {
+        SCTy = MinMax->getSCEVType();
+        LHS = MinMax->getOperand(0);
----------------
caojoshua wrote:
> unused var
it is used as an output of the function


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15033
+                                           const SCEV *Divisor) {
+      if (!isKnownNonNegative(Expr) || !isKnownPositive(Divisor))
+        return Expr;
----------------
mkazantsev wrote:
> If I'm reading this correctly, this is a guarantee of no-overflow for computations you are doing. Maybe add this comment explicitly?
I will only handle constants for now.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15038
+        // return the SCEV: Expr + Divisor - Expr % Divisor
+        return getAddExpr(getMinusSCEV(Divisor, Rem), Expr);
+      }
----------------
mkazantsev wrote:
> 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?
I will only support constants for now


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15038
+        // return the SCEV: Expr + Divisor - Expr % Divisor
+        return getAddExpr(getMinusSCEV(Divisor, Rem), Expr);
+      }
----------------
alonkom wrote:
> mkazantsev wrote:
> > 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?
> I will only support constants for now
I will only support constants for now


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15060
+    // Divisor.
+    std::function<const SCEV *(const SCEV *, const SCEV *)>
+        ApplyDivisibiltyOnMinMaxExpr = [&](const SCEV *MinMaxExpr,
----------------
mkazantsev wrote:
> auto
can't use auto since there's a recursive call here


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15121
+    // divisibilty information expressed as (A /u B) * B, while B is a constant.
+    // Return wether the divisibilty info was found, and the divisor B in \p
+    // DividesBy.
----------------
caojoshua wrote:
> typo: wether
> 
> I'm not sure what `the divisor B in \p DividesBy` means.
> 
> I think this paragraph needs to be more clear. What does it mean to be composed on Min/Max SCEVs?
rephrased it a bit.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15123
+    // DividesBy.
+    std::function<bool(const SCEV *, const SCEV *&)> HasDivisibiltyInfo =
+        [&](const SCEV *Expr, const SCEV *&DividesBy) {
----------------
mkazantsev wrote:
> auto
can't use auto since there's a recursive call here


================
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) {
----------------
mkazantsev wrote:
> auto
can't use auto, due to recursive call.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141850/new/

https://reviews.llvm.org/D141850