[PATCH] D23059: Teach CorrelatedValuePropagation to mark adds as no wrap
Sanjoy Das via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 2 10:40:04 PDT 2016
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
More information about the llvm-commits
mailing list