[PATCH] D22551: CodeGen: If Convert blocks that would form a diamond when tail-merged.

[Xinliang David Li via llvm-commits]

On Wed, Jul 20, 2016 at 2:31 PM, Kyle Butt <kyle+llvm at iteratee.net> wrote:

> iteratee added a comment.
>
> In https://reviews.llvm.org/D22551#490028, @davidxl wrote:
>
> > 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.
> >
> > 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.
>
>
> 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.
>


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).



>
> > 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.
>
>
> Two things:
>
> 1. The code has to count duplicates anyway. Look at how IfConvertDiamond
> is written.
> 2. There is a precedent for teaching optimization passes to look deeper,
> even if there is a canonicalization pass that would prevent the need.
>


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)

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.




>
> > 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?)
>
>
> Not specifically. If tailMerge is made less aggressive only during layout,
> then this is not necessary.
>
>
great.

thanks,

David


>
> https://reviews.llvm.org/D22551
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160720/eeacf065/attachment.html>