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

[Sanjoy Das]

sanjoy added a comment.

Second round of review comments inline.


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:587
@@ +586,3 @@
+    // (e.g. a nuw add) would trigger undefined behavior on overflow.
+    SCEV::NoWrapFlags getNoWrapFlagsFromPoison(const Value *V);
+
----------------
Minor & optional: I'd call this `getNoWrapFlagsFromUB`.

================
Comment at: include/llvm/Analysis/ValueTracking.h:313
@@ +312,3 @@
+  /// the loop header.
+  bool isGuaranteedToExecuteForEveryIteration(const Instruction *I,
+					      const Loop *L);
----------------
Is the indentation a little bit off here?

Actually, I'll just assume you'll run this change through `clang-format` before checking in, and won't make any further whitespace related comments.  Please let me know if you'd prefer otherwise.

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

================
Comment at: include/llvm/Analysis/ValueTracking.h:339
@@ +338,3 @@
+  /// the basic block that is the parent of I.
+  bool undefinedBehaviorIsGuaranteedIfFullPoison(const Instruction *PoisonI);
+
----------------
Similarly, how about calling this `isKnownNotPoison`?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4144
@@ +4143,3 @@
+      if (isLoopInvariant(OtherOp, AddRec->getLoop()) &&
+          isGuaranteedToExecuteForEveryIteration(BinOp, AddRec->getLoop()))
+        return Flags;
----------------
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.

================
Comment at: lib/Analysis/ValueTracking.cpp:3327
@@ +3326,3 @@
+    !isa<InvokeInst>(I) && // might not terminate
+    !isa<ResumeInst>(I) && // has no successors
+    !isa<ReturnInst>(I); // has no successors
----------------
If you're dealing with terminator instructions here (I'm not sure that that is necessary) why is `br` okay?  Can't a `br` form an infinite loop?


http://reviews.llvm.org/D11212