[PATCH] D11313: [ValueTracking] computeOverflowForSignedAdd and isKnownNonNegative

[Philip Reames via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/minor comments addressed.  No further review needed.


================
Comment at: include/llvm/Analysis/ValueTracking.h:303
@@ +302,3 @@
+                                             const DominatorTree *DT);
+  // This version also leverages the sign bit of Add if known.
+  OverflowResult computeOverflowForSignedAdd(AddOperator *Add,
----------------
/// doxygen comment please.

================
Comment at: include/llvm/Analysis/ValueTracking.h:306
@@ +305,3 @@
+                                             const DataLayout &DL,
+                                             AssumptionCache *AC,
+                                             const Instruction *CxtI,
----------------
AC, CtxI, and DT need to be optional and defaulted to nullptr.

================
Comment at: lib/Analysis/ValueTracking.cpp:3343
@@ +3342,3 @@
+
+  if (Add) {
+    // If the sign of Add is the same as at least one of the oeprands, this add
----------------
Use an early return to reduce nesting please.

================
Comment at: lib/Analysis/ValueTracking.cpp:3344
@@ +3343,3 @@
+  if (Add) {
+    // If the sign of Add is the same as at least one of the oeprands, this add
+    // CANNOT overflow. This is particularly useful when the sum is
----------------
operands

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:354
@@ -362,3 +353,3 @@
 bool NaryReassociate::maySignOverflow(AddOperator *AO, Instruction *Ctxt) {
   if (AO->hasNoSignedWrap())
     return false;
----------------
Doesn't need to be in this change, but this should be pushed inside the ValueTracking call and the wrapper function can probably completely disappear.  


http://reviews.llvm.org/D11313