<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Jul 11, 2016 at 7:12 PM Xinliang David Li <<a href="mailto:xinliangli@gmail.com">xinliangli@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"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jul 11, 2016 at 6:55 PM, Chandler Carruth via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">chandlerc added inline comments.<br>
<span><br>
================<br>
Comment at: lib/CodeGen/BranchFolding.cpp:656<br>
@@ +655,3 @@<br>
+  // duplicated.<br>
+  if (!AfterPlacement)<br>
+    if (SuccBB && MBB1 != PredBB && MBB2 != PredBB &&<br>
----------------<br>
</span><span>davidxl wrote:<br>
> iteratee wrote:<br>
> > Discussed this with chandlerc. He thinks that passing around the list of blocks that were tail-duplicated too tightly couples the passes, and that we should strive to have non-overlap. He suggested that a list of blocks that were duplicated might be a good debug measure to make sure they aren't being re-merged.<br>
> ><br>
> > Overall I agree with him, and I made a change so there shouldn't be any overlap.<br>
> > Do you want to see the set as a debugging measure?<br>
> I don't agree. Without making it explicit, the the pass coupling does not disappear -- it is still there but in a more implicit but hard to maintain way.<br>
><br>
> For instance, can we guarantee this still work when TailMerger and TailDuplicator uses non-default size thresholds?<br>
</span>My thought was specifically that we should enforce an invariant that these are defined in a non-overlapping way, *and* check it in asserts builds precisely so that things like non-default size thresholds work in a sane way.<br>
<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>The problem is that the implementation maintains the non-overlapping by making assumption about the size limit (the default setting), so when non-default size is used, the assertion will likely to fire off.</div></div></div></div></blockquote><div><br></div><div>Having two thresholds like this that are coupled is definitely problematic, but it seems like the solution should be to have a single threshold rather than two.</div><div><br></div><div>Put differently, whether we are merging or duplicating tails, we should have one threshold and maybe a tolerance that provides any desired padding between the two operations. That way it will be impossible to configure this in an overlapping manner.</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 class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
As long as there is the potential for overlap, we have the problem (IMO) that the innards of tail dup or tail merge can do things that user programs cannot. I would much prefer that we have a set of rules that are sufficiently precise and enforced that one pass doesn't have to have a back-channel to communicate to the other pass.<br>
<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Passing a 'filter' is actually the exact mechanism IMO to hide pass implementation details from one another. Is there a better way? Making any assumption about how tailDup is done in tailMerger is the wrong direction to take.</div></div></div></div></blockquote><div><br></div><div>This assumes that all duplication of tail code sequences is done by a particular part of the compiler. The input program may have manually done such duplication itself. Same story for merging.</div><div><br></div><div>The behavior shouldn't change when you manually apply a transform to the source program rather than having LLVM apply it for you.</div><div><br></div><div>For that to work, we can't rely on communicating between the two systems, we need to actually define the operations in non-overlapping ways. We do the exact same thing for many other code motion transforms where we will transform in both directions to try and converge in the middle.</div><div><br></div><div><br></div><div>But all of this is not to say that the problem of having two thresholds with a subtle coupling isn't a real problem! It is. We should define the interfaces in a way that clearly surfaces that there is some point above which we will merge and below which we will duplicate and that it is one point (not two) with maybe some delta around the point where we will make no change at all.</div></div></div>