<div dir="ltr">Also note, it's possible to have a case where root.start dominates but it is not safe to propagate due to multiple edges from that block to to root.getEnd.<div>The whole reason for having the edge is to avoid this case, AFAIK.</div><div><br></div><div>So i'm pretty sure using root.getStart() is just going to cause incorrect transforms.</div><div><br></div><div>Note that all of propagateEquality is really somewhat of a hack to avoid using proper control dependence, and at some point, we'll likely want to just rip it out and use post-dominators to do this *right*.</div><div><br></div><div>I don't know whether the optimization capability we lose here by avoiding assume in some cases makes us want to do that more (I leave that question to data and chandler)</div><div><br></div><div><br><div><div> </div></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 12, 2016 at 11:07 AM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">dberlin added inline comments.<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/Local.h:320<br>
@@ -320,2 +319,3 @@<br>
+/// the end of the given BasicBlock. Returns the number of replacements made.<br>
 unsigned replaceDominatedUsesWith(Value *From, Value *To, DominatorTree &DT,<br>
                                   const BasicBlock *BB);<br>
----------------<br>
I'm not sure this wording change makes sense.<br>
<br>
It doesn't care whether it's the end of the block that dominates or not.<br>
And by definition, uses in the same-block as their definitions must be dominated by those defs.<br>
<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/GVN.cpp:2137<br>
@@ +2136,3 @@<br>
+/// If DominatesByEdge is false, it means that there might be multiple edges<br>
+/// from Root.Start to Root.End, in which case we need to pass Root.Start and<br>
+/// use a different implementation of replaceDominatedUsesWith to test.<br>
----------------<br>
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).<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/GVN.cpp:2201<br>
@@ -2197,3 +2200,3 @@<br>
               ? replaceDominatedUsesWith(LHS, RHS, *DT, Root)<br>
-              : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getEnd());<br>
+              : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getStart());<br>
<br>
----------------<br>
If this problem only occurs with assume instructions, we shouldn't change the generic code.<br>
If it's not a problem that only occur with assume instructions, we need<br>
A. more testcases.<br>
B. a more complete description of the problem that is happening and why this is the correct fix.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/GVN.cpp:2277<br>
@@ -2273,3 +2276,3 @@<br>
                   : replaceDominatedUsesWith(NotCmp, NotVal, *DT,<br>
-                                             Root.getEnd());<br>
+                                             Root.getStart());<br>
           Changed |= NumReplacements > 0;<br>
----------------<br>
Same comment as above :)<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/Local.cpp:1540<br>
@@ -1539,3 +1539,3 @@<br>
     auto *I = cast<Instruction>(U.getUser());<br>
-    if (DT.dominates(BB, I->getParent())) {<br>
+    if (DT.properlyDominates(BB, I->getParent())) {<br>
       U.set(To);<br>
----------------<br>
This is definitely not correct.<br>
Local to a block, all uses are dominated by their definition, so you don't need properlyDominates to replace them legally.<br>
It's up to the caller to determine whether this is safe *semantically*.<br>
<br>
(Even if this change was correct, you've also now made the replacement functions completely inconsistent with each other.)<br>
<br>
<br>
<a href="http://reviews.llvm.org/D16100" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16100</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>