<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Jan 12, 2016 at 1:36 PM Piotr Padlewski <<a href="mailto:piotr.padlewski@gmail.com">piotr.padlewski@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">@Daniel: I think Ray is right, because I remember that you had pretty the same concerns when I did the my patch for this: <a href="http://reviews.llvm.org/D12170" target="_blank">http://reviews.llvm.org/D12170</a>. Unfortunatelly I think not all of your comments have made it to the review, but I am pretty sure that you were worried about the case with the switch, and the patch was right. </div></blockquote><div>I believe it is right *for assume*, which was the conclusion on that thread.<br></div><div><br></div><div>But "dominatedbyEdge" is not a flag that tells us we are just caring about assume :)</div><div><br>It makes it sound like it will work in other cases as well, and ... it won't :)</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Like I said, I need more time to read it again and try to recall it, but this patch seems promising.</div></blockquote><div><br></div><div>I'm actually pretty convinced the patch will fix the issue. My concern at this point is that this function is getting immensely confusing and complicated to follow given the weird cases it now handles.</div><div>It also pretends to be more general than it is.</div><div><br></div><div><br></div><div>Certainly, i would not expect anyone who has not an amazingly detailed understanding of GVN to be able to understand how to fix or debug this function.</div><div><br></div><div>That is ... concerning.</div><div>At the point at which it is doing very different CFG tests for very different cases, i'd rather see it split or something.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>Piotr</div></div><div class="gmail_extra"><br><div class="gmail_quote">2016-01-12 22:21 GMT+01:00 Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>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>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>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></span><div>This seems just wrong, for the reasons you've pointed out.</div><span><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><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>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> ? 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></span><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><span><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></span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><span>
bool RootDominatesEnd = DT.dominates(Root.Start(), Root.End()), then it can be used:<br></span></blockquote><div><br></div><div>This will hit the multiple edge case, sadly.</div><span><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></span><div>I'm not sure this is worth the confusion/cost.</div><div><br></div><div>This seems *super* complicated.</div><span><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></span><div>It's safe, but confusing.</div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><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>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></span><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>
</blockquote></div><br></div>
</blockquote></div></div>