<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 20, 2016 at 2:31 PM, Kyle Butt <span dir="ltr"><<a href="mailto:kyle+llvm@iteratee.net" target="_blank">kyle+llvm@iteratee.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">iteratee added a comment.<br>
<span class=""><br>
In <a href="https://reviews.llvm.org/D22551#490028" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22551#490028</a>, @davidxl wrote:<br>
<br>
> I am torn about this change. While this looks like a useful thing to do, I suspect this is not the right way to approach the problem.<br>
><br>
> The example actually confirms the fact the pre-layout tailmerging is a good normalization/enabler pass for later optimizations. This is the reason why it should be run with lower threshold enabling as much optimization as possible, and later let TailDup to undo those that do not bring benefit and to improve layout.<br>
<br>
<br>
</span>I couldn't find any of the original commits about tail merging that refer to normalization. It's always about reducing code size. Also, it's unusual for a canonicalization pass to have a threshold.<br></blockquote><div><br></div><div><br></div><div>I don't think this argument holds well :) For instance, I don't think LICM or GEP merge etc started out as normalization pass, but they are used for that purpose (for better analysis with simplified dependence analysis, alias analyse control flow etc).</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> Another question is that whether this patch can handle more cases where (tailMerge + ifcvt) can not handle. If not, it seems to me the patch seems to have duplicated logics (e.g, counting dups) in tailMerge which is not the right approach.<br>
<br>
<br>
</span>Two things:<br>
<br>
1. The code has to count duplicates anyway. Look at how IfConvertDiamond is written.<br>
2. There is a precedent for teaching optimization passes to look deeper, even if there is a canonicalization pass that would prevent the need.<br></blockquote><div><br></div><div><br></div><div>I am not opposing to teaching the optimization pass to look deeper (and I am aware of some recent change), but there is a balance. It is always about cost and benefit.  In your case, it can easily seen that the cost (long term maintenance) is high (with > 500 LOC added), but the *actual* benefit is low -- it does not add something existing optimization pipeline does not already do.   With tailMerging turned on, your code most likely won't be exercised in reality (other than the test where tailmerge is explicitly disabled)</div><div><br></div><div>On the other hand, is it possible that you can refactor the code to make the analysis sharing as much as possible existing code? If that is done, I will feel much more comfortable supporting it.</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> Is this patch required to enable your tailDup enhancement patch? I don't think this one is essential for it.   We can probably focus on getting your tailDup patch in first (it is very close to get -- probably just to make your latest tailMerge tuning  to be enabled only in post layout mode?)<br>
<br>
<br>
</span>Not specifically. If tailMerge is made less aggressive only during layout, then this is not necessary.<br>
<br></blockquote><div><br></div><div>great.</div><div><br></div><div>thanks,</div><div><br></div><div>David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a href="https://reviews.llvm.org/D22551" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22551</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>