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

[Bjarke Hammersholt Roune via llvm-commits]

broune marked 4 inline comments as done.

================
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
----------------
sanjoy wrote:
> 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`.
I changed it to (-1)*RHS.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:3408
@@ +3407,3 @@
+      // not know that in this case.
+      if (RHSIsNotMinSigned || isa<SCEVAddRecExpr>(RHS))
+        NegFlags = SCEV::FlagNSW;
----------------
sanjoy wrote:
> 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.
Good point, I added a test case that is based on this. It could also be an addrec on the outer loop, which is what I was thinking of it as, but your example shows that that interpretation is not sufficient. There should be a workable scheme where whomever proves NSW or NUW and passes in those flags also can explicitly pass in the loop that the flags are proven within, though I'll leave that for another time.

I now only put NSW on the negation if it can be proven without using the passed-in NSW.


http://reviews.llvm.org/D11860