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

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3381
@@ +3380,3 @@
+    // Let M be the minimum representable signed value. Then -RHS
+    // signed-wraps if and only if RHS is M. That can happen even for
+    // a NSW subtraction because e.g. -M signed-wraps even though -1 - M
----------------
My subjective opinion is that since there is no direct representation of `-X` in LLVM IR, we ultimately have to choose a specific representation to reason about.  The specific representation is not obvious either since both `sub 0 X` and `mul -1 X` are equally valid, so it is nice to be explicit about these things.  Moreover, here things are doubly confusing since `sub 0 X` and `mul -1 X` have identical behavior w.r.t. `nsw` so there is no easy way for the reader to "check" if (s)he understood the intent correctly.

However, objectively, I don't think there is any problem with directly proving things about a mathematical negation operation, so I'd suggest just putting in a sentence either here or above on what exactly you mean by `-RHS`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3408
@@ +3407,3 @@
+      // not know that in this case.
+      if (RHSIsNotMinSigned || isa<SCEVAddRecExpr>(RHS))
+        NegFlags = SCEV::FlagNSW;
----------------
But what if you have a loop nested within other, and LHS is an addrec on the inner loop while RHS is an addrec on the outer loop?  Then `LHS - RHS` would be an addrec on the inner loop (so the `nsw` is guaranteed to apply only on the inner loop), but `RHS` has a larger scope than the inner loop while still being an add rec.

IOW, something like this:

```
define void @f(i32 %outer_l, i32 %inner_l) {
 entry:
  br label %outer

 outer:
  %o_idx = phi i32 [ 0, %entry ], [ %o_idx.inc, %outer.be ]
  %o_idx.inc = add i32 %o_idx, 1
  %cond = icmp eq i32 %o_idx, 42
  br i1 %cond, label %inner, label %outer.be

 inner:
  %i_idx = phi i32 [ 0, %outer ], [ %i_idx.inc, %inner ]
  %i_idx.inc = add i32 %i_idx, 1
  %v = sub nsw i32 %i_idx, %o_idx
  %cond2 = icmp eq i32 %i_idx, %inner_l
  br i1 %cond2, label %outer.be, label %inner

 outer.be:
  %cond3 = icmp eq i32 %o_idx, %outer_l
  br i1 %cond3, label %exit, label %outer

exit:
 ret void
}
```

where `%v` is an add rec for the `%inner` loop while `%o_idx` is an add rec on the outer loop.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4135
@@ -4098,2 +4134,3 @@
+  if (!BinOp) return SCEV::FlagAnyWrap;
 
   // Return early if there are no flags to propagate to the SCEV.
----------------
Then I'd suggest `if (isa<ConstantExpr>(V)) return FlagAnyWrap;`, that makes the code more obvious.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4428
@@ -4372,3 +4427,3 @@
         APInt::getOneBitSet(BitWidth, SA->getZExtValue()));
-      return getMulExpr(getSCEV(U->getOperand(0)), getSCEV(X));
+      return getMulExpr(getSCEV(U->getOperand(0)), getSCEV(X), Flags);
     }
----------------
SGTM.


http://reviews.llvm.org/D11860