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

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Comments inline.


================
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) {
----------------
Any non-zero value except 1. :)

================
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
----------------
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).

================
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
----------------
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`?

================
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;
----------------
Why is this needed?  Aren't we ever only going to pass in `mul`, `sub` and `shl`?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4237
@@ -4212,10 +4236,3 @@
       // formed with operands from AddOps.
-      //
-      // FIXME: Expand this to sub instructions.
-      if (Opcode == Instruction::Add && isa<BinaryOperator>(U)) {
-        SCEV::NoWrapFlags Flags = getNoWrapFlagsFromUB(U);
-        if (Flags != SCEV::FlagAnyWrap) {
-          AddOps.push_back(getAddExpr(getSCEV(U->getOperand(0)),
-                                      getSCEV(U->getOperand(1)), Flags));
-          break;
-        }
+      const SCEV *RHS = getSCEV(U->getOperand(1));
+      SCEV::NoWrapFlags Flags = getNoWrapFlagsFromUB(U);
----------------
Why not hoist `LHS = getSCEV(U->getOperand(0)` as well?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4265
@@ +4264,3 @@
+
+      if (auto *OpSCEV = getExistingSCEV(Op)) {
+        MulOps.push_back(OpSCEV);
----------------
I'll be in favor of using `U` here instead of `Op` to be consistent, unless that does not work for some reason.

================
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));
----------------
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


http://reviews.llvm.org/D11860