[PATCH] D11918: Constant propagation after hiting assume(icmp)

[Daniel Berlin via llvm-commits]

dberlin added a comment.

So, this is essentially trying to start to get GVN to recognize that operations are made of operations on value numbers.

Except you are only handling the really special case where those value numbers are  constant over the entire program (instead of also handling cases where the operations simplify, or have arithmetic niceties, etc. SimplifyInstruction will do some of this, but it won't notice some classes of things)

I'm generally biased against trying to bolt more into our current GVN, but hey, when you do stuff like this, it just makes New GVN a lot faster by comparison :)

So SGTM


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2231
@@ +2230,3 @@
+      if (ICmpInst *ICmpI = dyn_cast<ICmpInst>(V)) {
+        if (ICmpI->getSignedPredicate() == ICmpInst::Predicate::ICMP_EQ) {
+
----------------
Why only ICMP_EQ?

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2245
@@ +2244,3 @@
+                // equality propagation can't be done for every
+                // successor, but propagateEquality checks it
+                Changed |= propagateEquality(Lhs,
----------------
nlewycky wrote:
> Period.
This isn't quite right :)
There are cases that propagate equality checks, and cases we don't call it because it would do the wrong thing.

For example, i know your code doesn't quite work right with assume followed by certain types of switch statements (where there are multiple edges to the same successor).

See the code handling switch statements, that verifies there are not multiple edges to the same successor, and if there are not, *does not call propagateEquality* (old line 2259)

You are going to either need to unify or duplicate a lot of this kind of logic, depending on the terminator of the basic block the assume statement is in.




http://reviews.llvm.org/D11918