[PATCH] D23059: Teach CorrelatedValuePropagation to mark adds as no wrap

[Sanjoy Das via llvm-commits]

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

lgtm modulo comments inline


================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:21
@@ -20,2 +20,3 @@
 #include "llvm/IR/CFG.h"
+#include "llvm/IR/ConstantRange.h"
 #include "llvm/IR/Constants.h"
----------------
Interesting that you had to add this.  Doesn't LVI include this?

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:385
@@ -383,1 +384,3 @@
 
+static bool processAdd(BinaryOperator *AI, LazyValueInfo *LVI) {
+  typedef OverflowingBinaryOperator OBO;
----------------
Why `AI` as the variable name?  I'd have gone with `BO` or something like that.

edit: I suppose you mean `AddInst`?  In that case, I'd suggest calling it `AddInst` or `AddOp`.  I've seen `AI` be normally used for `alloca` instructions.

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:409
@@ +408,3 @@
+                                              OBO::NoUnsignedWrap);
+    bool NewNUW = NUWRange.contains(RRange);
+    AI->setHasNoUnsignedWrap(NewNUW);
----------------
There is an optimization we can do here:  we only need `RRange` if `NUWRange` (resp. `NSWRange`) is not empty.  What do you think about changing `RRange` to an `Optional<ConstantRange>` and lazily initializing it?  My intuition is that calling `getConstantRange` on a value that LVI hasn't seen before can be expensive (since it will have to do a CFG-walk).

================
Comment at: test/Transforms/CorrelatedValuePropagation/add.ll:51
@@ +50,3 @@
+entry:
+  %cmp = icmp ule i32 %a, -1
+  br i1 %cmp, label %bb, label %exit
----------------
For completeness, can you please add a few tests like `@test3` and `@test4` with signed integer min and signed integer max?


https://reviews.llvm.org/D23059