[PATCH] D16100: Fix for two constant propagation problems in GVN with assume intrinsic instruction

[Daniel Berlin via llvm-commits]

dberlin added inline comments.

================
Comment at: include/llvm/Transforms/Utils/Local.h:320
@@ -320,2 +319,3 @@
+/// the end of the given BasicBlock. Returns the number of replacements made.
 unsigned replaceDominatedUsesWith(Value *From, Value *To, DominatorTree &DT,
                                   const BasicBlock *BB);
----------------
I'm not sure this wording change makes sense.

It doesn't care whether it's the end of the block that dominates or not.
And by definition, uses in the same-block as their definitions must be dominated by those defs.



================
Comment at: lib/Transforms/Scalar/GVN.cpp:2137
@@ +2136,3 @@
+/// If DominatesByEdge is false, it means that there might be multiple edges
+/// from Root.Start to Root.End, in which case we need to pass Root.Start and
+/// use a different implementation of replaceDominatedUsesWith to test.
----------------
If there are multiple edges to the same BB, we should not do the replacement at all (we lack post-dominance info necessary to be safe in that case).

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2201
@@ -2197,3 +2200,3 @@
               ? replaceDominatedUsesWith(LHS, RHS, *DT, Root)
-              : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getEnd());
+              : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getStart());
 
----------------
If this problem only occurs with assume instructions, we shouldn't change the generic code.
If it's not a problem that only occur with assume instructions, we need
A. more testcases.
B. a more complete description of the problem that is happening and why this is the correct fix.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2277
@@ -2273,3 +2276,3 @@
                   : replaceDominatedUsesWith(NotCmp, NotVal, *DT,
-                                             Root.getEnd());
+                                             Root.getStart());
           Changed |= NumReplacements > 0;
----------------
Same comment as above :)

================
Comment at: lib/Transforms/Utils/Local.cpp:1540
@@ -1539,3 +1539,3 @@
     auto *I = cast<Instruction>(U.getUser());
-    if (DT.dominates(BB, I->getParent())) {
+    if (DT.properlyDominates(BB, I->getParent())) {
       U.set(To);
----------------
This is definitely not correct.
Local to a block, all uses are dominated by their definition, so you don't need properlyDominates to replace them legally.
It's up to the caller to determine whether this is safe *semantically*.

(Even if this change was correct, you've also now made the replacement functions completely inconsistent with each other.)


http://reviews.llvm.org/D16100