[PATCH] D11860: [SCEV] Apply NSW and NUW flags via poison value analysis for sub, mul and shl

[Bjarke Hammersholt Roune via llvm-commits]

broune added inline comments.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3378
@@ +3377,3 @@
+  // We will transform LHS - RHS to LHS + (-RHS). Thus we cannot make
+  // use of NUW, since -RHS will unsigned-wrap for any non-zero value.
+  if (maskFlags(Flags, SCEV::FlagNSW) == SCEV::FlagNSW) {
----------------
sanjoy wrote:
> Any non-zero value except 1. :)
It's true that getNegativeSCEV happens to represent -RHS as (-1) * RHS which as an unsigned operation would look like MAX * RHS which indeed doesn't wrap for RHS==1. My thinking is that the mathematical operation of negation does wrap for RHS==1, as you end up with MAX instead of -1. So as I think of it, it would be incorrect to pass the NUW flag to getNegativeSCEV even if you knew that RHS == 1, as the mathematical operation is not NUW, but it would be correct for getNegativeSCEV itself to recognize if RHS==1 and apply the NUW flag in that case (ignoring that it could fold to a constant in that case).

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3380
@@ +3379,3 @@
+  if (maskFlags(Flags, SCEV::FlagNSW) == SCEV::FlagNSW) {
+    // Let M be the minimum representable signed value. Then -RHS
+    // signed-wraps if and only if RHS is M. That can happen even for
----------------
sanjoy wrote:
> I'm being pedantic here, but the reasoning here may be easier to follow if you write `-Foo` as `-1 * Foo` (assuming that's what you mean).
-Foo is the mathematical operation being done, while (-1) * Foo is the representation that getMinusSCEV happens to choose for that operation (at least before simplification). In the way that I'm thinking of this, the flags that are passed in to getAddExpr and getNegativeSCEV concern the mathematical operation, not the representation, so the comment follows the math.

I could change this if you think that the passed-in flags should concern the representation, instead?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3396
@@ +3395,3 @@
+      // while the RHS has no recurrence. If RHS has no recurrence and
+      // we attach NSW to the SCEV for -RHS, then we are stating that
+      // -RHS never wraps anywhere, but we only know that -RHS does
----------------
sanjoy wrote:
> I did not quite follow the logic here -- what's is the "relevant loop" here?  An example will be very useful here.
> 
> Also, why don't you need `cast<SCEVAddRecExpr>(RHS)->getLoop() == RelevantLoop`?
I added a comment with an example. The difficulty that this comes out of is that flags and many SCEV operations do not have a loop/scope attached to them, only recurrences do. So we have to be careful with applying flags to subexpressions that do not involve a recurrence. In this case, the relevant loop would be the one from the recurrence for the purposes of this check.

I also realized that this check is unnecessary if NSW was proven by looking only at RHS on its own, with no use of LHS or the Flags parameter, so I added a check for that case.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4123
@@ -4096,2 +4122,3 @@
 SCEV::NoWrapFlags ScalarEvolution::getNoWrapFlagsFromUB(const Value *V) {
-  const BinaryOperator *BinOp = cast<BinaryOperator>(V);
+  const BinaryOperator *BinOp = dyn_cast<BinaryOperator>(V);
+  if (!BinOp) return SCEV::FlagAnyWrap;
----------------
sanjoy wrote:
> Why is this needed?  Aren't we ever only going to pass in `mul`, `sub` and `shl`?
This function can be passed a `ConstantExpr` version of those, which it cannot handle. I preferred to check for that once here instead of doing it in each caller.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4408
@@ -4372,2 +4407,3 @@
         APInt::getOneBitSet(BitWidth, SA->getZExtValue()));
-      return getMulExpr(getSCEV(U->getOperand(0)), getSCEV(X));
+      return getMulExpr(getSCEV(U->getOperand(0)), getSCEV(X),
+                        getNoWrapFlagsFromUB(U));
----------------
sanjoy wrote:
> Unfortunately, this is not quite right.  There are some inconsistencies in the langref that to my knowledge have not yet been fixed:  see http://lists.llvm.org/pipermail/llvm-dev/2015-April/084195.html and http://reviews.llvm.org/D8890
It seems that there is agreement for shl nsw to be equivalent to mul nsw, in which case this is correct, except that no one has bothered to update the LangRef yet, so it's not official. Also, some updates in InstCombine (and possibly elsewhere) would be required to make the change and those also haven't been done yet. Is that right? I added a comment to explain this, along with a check to avoid applying flags for left shift by BitWidth - 1 until the situation is resolved.


http://reviews.llvm.org/D11860