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

[Sanjoy Das]

sanjoy accepted this revision.
sanjoy added a comment.

Some very minor non-semantic nits inline, otherwise looks good to me.  Feel free to check in as is and fix the nits in a follow up change if you find that more convenient.


================
Comment at: include/llvm/Analysis/ValueTracking.h:293
@@ +292,3 @@
+
+  /// Return true if this function can prove that the instruction I will
+  /// always transfer execution to one of its successors (including the next
----------------
Please fix this in the comments for the other functions you added as well.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4111
@@ +4110,3 @@
+  // containing BinOp, since we only deal with instructions in the loop
+  // 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
----------------
Nit: change "rec" to something more descriptive, perhaps "add recurrence"?

================
Comment at: lib/Analysis/ValueTracking.cpp:3326
@@ +3325,3 @@
+         !isa<CallInst>(I) &&    // could throw and might not terminate
+         !isa<InvokeInst>(I) &&  // might not terminate
+         !isa<ResumeInst>(I) &&  // has no successors
----------------
Minor nit: `invoke`s can also throw (see https://llvm.org/bugs/show_bug.cgi?id=24185, especially https://llvm.org/bugs/show_bug.cgi?id=24185).  Btw, I got the idea for testing for 24185 bug from reading this function.


http://reviews.llvm.org/D11212