<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 12, 2016 at 1:02 PM, Yuanrui Zhang <span dir="ltr"><<a href="mailto:yuanrui.zhang@intel.com" target="_blank">yuanrui.zhang@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Ray added inline comments.<br>
<br>
================<br>
<span class="">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>
</span><span class="">dberlin wrote:<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>
</span>Note that, there is a condition about "multiple edges to the same BB": it is from Root.Start to Root.End. That's when DominatesByEdge is set to false, and that's exactly the reason why DominatesByEdge was introduced as a parameter to this function. See the example added before<br></blockquote><div><br></div><div>This seems just wrong, for the reasons you've pointed out.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
I was trying to make the comments in more detail. But if it is confusing, we can improve it.<br>
<span class=""><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>
</span><span class="">dberlin wrote:<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>
</span>===This is a general problem in GVN::propagateEquality with the following code:<br>
<br>
unsigned NumReplacements =<br>
DominatesByEdge<br>
<span class=""> ? replaceDominatedUsesWith(LHS, RHS, *DT, Root)<br>
</span> : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getStart());<br>
<br>
When DominatesByEdge is TRUE, it passes the edge "Root" to replaceDominatedUsesWith, and we are fine. Since, if the edge "Root" dominates a USE, then Root.Start must dominates the USE.<br></blockquote><div><br></div><div>Yes, it must, but that does not make it safe :)<br></div><div><br></div><div>If you propagate something through a switch statement, for example, it will have different cases that go to the same block, but you can't replace things in those blocks :)</div><div><br></div><div>THat is, root.getStart will dominate all the uses, but you can't do *any* replacement.</div><div>root.getEnd will not dominate all the uses, only the ones actually in the target edge end.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
When DominatesByEdge is FALSE, it passes the Root.End to replaceDominatedUsesWith, and we have a problem here. Since, if Root.End dominates a USE, it does not mean that Root.Start dominates the USE. We show in the test case _Z1im below.<br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
bool RootDominatesEnd = DT.dominates(Root.Start(), Root.End()), then it can be used:<br></blockquote><div><br></div><div>This will hit the multiple edge case, sadly.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In fact, the simplest way is to<br>
1) pass Root.Start to replaceDominatedUsesWith,<br>
2) then change replaceDominatedUsesWith(... BasicBlock* BB) to replace the uses by the "END" of the BB.<br>
These two changes together will give exactly the same effect as when passing an edge, i.e. ,replaceDominatedUsesWith(..... BasicBlockEdge Root).<br>
<br></blockquote><div>I'm not sure this is worth the confusion/cost.</div><div><br></div><div>This seems *super* complicated.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Note that, currently GVN is the only caller of replaceDominatedUsesWith( ..., BasicBlock* BB). So, it is safe to change to use "properlyDominates".<br>
<br></blockquote><div><br></div><div>It's safe, but confusing.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><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>
</span><span class="">dberlin wrote:<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>
</span>Since propagateEquality in GVN is the only caller, that's why we changed the comments to inform other users that, this function will replace USES from the END of the BB.<br>
<br>
If the called wants to replace the uses in the BB itself, the other overloaded interface replaceDomiantedUsesWith(..... BasicBlockEdge Root) can be used.<br></blockquote><div><br></div><div><br></div><div>I'd like to hear Piotr's thoughts on all of this.</div><div><br>My view is that if we have to make propagateEquality this complicated to support assume, we should seriously reconsider how we do propagateEquality (or split propagateEquality into propagateEquality and propagateAssume, or *something*).</div><div><br>What is being done here is just making an already hard-to-understand function *much* harder to understand.</div><div><br></div></div></div></div>