[PATCH] D11212: [SCEV] Apply NSW and NUW flags via poison value analysis

[Sanjoy Das]

sanjoy added a comment.

Minor nits inline.  At this point I think this change is ready to go in once the style / naming fixes are done.  However:

- I'd like to take a look at the final change before it goes in.
- I'd also like Andy to take a look before this goes in.

Side comment and optional: have you bootstrapped clang with this change?  That's a good sanity check for this sort of change.  You may consider bootstrapping with ubsan / asan too to get some extra coverage:  the extra control flow the sanitizers add tends to shake out a lot of bugs.


================
Comment at: include/llvm/Analysis/ValueTracking.h:293
@@ +292,3 @@
+
+  /// Returns true if this function can prove that the instruction I will
+  /// always transfer execution to one of its successors (including the next
----------------
This is borderline bikeshedding, but the rest of the file specifies behavior as a verb -- `Return true if ...`

================
Comment at: include/llvm/Analysis/ValueTracking.h:311
@@ +310,3 @@
+  ///
+  /// Note that this is currently based on an analysis that only considers
+  /// the loop header.
----------------
Minor: I'd just say "Note that this currently only looks at the loop header".  Specifically, I'd avoid using the term "analysis" since that has a specific meaning within LLVM.

================
Comment at: include/llvm/Analysis/ValueTracking.h:327
@@ +326,3 @@
+
+  /// Returns either nullptr or an operand of I such that I will trigger
+  /// undefined behavior if I is executed and that operand has a full-poison
----------------
broune wrote:
> sanjoy wrote:
> > I'd prefer if you were a little more terse -- perhaps `getGuaranteedNonPoisonOp`?
> > 
> > (IOW, which operand is guaranteed to not be poison since if it were poison then the program is undefined)
> I wasn't so happy with it myself and I like your suggestion. Maybe getGuaranteedNonFullPoisonOp ? I'll change it Monday.
SGTM.

================
Comment at: include/llvm/Analysis/ValueTracking.h:339
@@ +338,3 @@
+  /// the basic block that is the parent of I.
+  bool undefinedBehaviorIsGuaranteedIfFullPoison(const Instruction *PoisonI);
+
----------------
broune wrote:
> sanjoy wrote:
> > Similarly, how about calling this `isKnownNotPoison`?
> Nice. isKnownNotFullPoison? I'm concerned that it would be too easy to miss the distinction between poison and full-poison and the bugs from that would be hard to shake out.
SGTM.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4113
@@ +4112,3 @@
+  // header. The actual loop we need to check later will come from a rec, but
+  // getting that requires computing the scev of the operands, which can be
+  // expensive. This check we can do cheaply to rule out some cases early.
----------------
Nitpick: elsewhere you use SCEV not "scev".

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4144
@@ +4143,3 @@
+      if (isLoopInvariant(OtherOp, AddRec->getLoop()) &&
+          isGuaranteedToExecuteForEveryIteration(BinOp, AddRec->getLoop()))
+        return Flags;
----------------
broune wrote:
> sanjoy wrote:
> > broune wrote:
> > > sanjoy wrote:
> > > > I'm not sure that this is correct.  I think you need to prove that the instruction you used to justify UB if `V` was poison is what needs to execute on every loop iteration.
> > > > 
> > > > This is not a problem currently because `undefinedBehaviorIsGuaranteedIfFullPoison` only looks at a single basic block, but semantically, the following program will be problematic:
> > > > 
> > > > ```
> > > > 
> > > >  loop_header:
> > > >    %x = add nsw %p, %q
> > > >    ...
> > > > 
> > > >  outside_the_loop:
> > > >    ;; no other uses of %x
> > > >    store i32 42, GEP(@global, %x)
> > > > 
> > > > ```
> > > > 
> > > > You can conclude that the `%x` does not overflow in the last iteration, but that's it -- even if `%x` was poison in all the other iterations your program is well defined.
> > > As you point out, UB is not guaranteed on poison in this example, so even if `undefinedBehaviorIsGuaranteedIfFullPoison` is made stronger by considering more basic blocks, it should still return false here, right?
> > > 
> > > 
> > In that case why do we bother with `isGuaranteedToExecuteForEveryIteration`?
> > 
> > IOW, why not
> > 
> > ```
> >   if (undefinedBehaviorIsGuaranteedIfFullPoison(BinOp))
> >     return Flags;
> > 
> > ```
> > 
> That would prove that if BinOp is executed, then what it calculates will not wrap. There is then still the possibility that BinOp is not executed on a given iteration, in which case we have no information about wrapping of the SCEV for that iteration. Then we cannot apply the flag to the SCEV as other instructions in the loop that map to the same SCEV would then also get the flag on the shared SCEV, but we have not actually proven that the shared SCEV does not wrap.
Ah, right.  You also have a large comment explaining the very same thing -- sorry for making you repeat yourself.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4207
@@ +4206,3 @@
+
+      // Do an operation by itself if a no-wrap flag can be applied, since the
+      // flag only applies to that particular operation.
----------------
The `Do an operation by itself if a no-wrap flag can be applied` bit did not parse for me.

================
Comment at: lib/Analysis/ValueTracking.cpp:3397
@@ +3396,3 @@
+    if (OBO->hasNoUnsignedWrap() || OBO->hasNoSignedWrap()) {
+      for (int OpIndex = 0; OpIndex < 2; ++OpIndex) {
+        if (auto *CI = dyn_cast<ConstantInt>(OBO->getOperand(OpIndex))) {
----------------
Can't you use `for (Value *V : OBO->operands())` here?


http://reviews.llvm.org/D11212