[PATCH] D22778: Add Loop Sink pass to reverse the LICM based of basic block frequency.

Fri Aug 5 08:20:43 PDT 2016

Assume literally all of that is 100% correct, and it should never change.
The first part of what i said is still true.

The approach of using AliasSetTracker stil makes this something you do not
want to run over the entire function.
In fact, as written, your pass should *already* have the same issue as
https://llvm.org/bugs/show_bug.cgi?id=28832, and be slow on the same
testcase

(there is a larger testcase for that where it will take about 600+ seconds,
instead of 100+)

As I said, you have have no way of degrading this gracefully, which is
meh'ish for loops, but much worse if you want it to be the general IR-level
sinking pass.

So i'm going to stand by what i said: "If you run this on the entire
function, you don't want this approach".  This is true no matter what the
purpose is.

On Thu, Aug 4, 2016 at 9:39 PM, Xinliang David Li <davidxl at google.com>
wrote:

> This is intended to be an centralized pass (not unified algorithm) to
> do a whole bunch of very specialized code sinking, which is different
> from anticipation based redundancy (store) elimination, nor is it
> limited to stores. In fact, the name 'sinking' is misleading. It is
> more a general forward propagation pass (for instructions that have
> multiple uses in multiple BBs) to reduce overall frequency or to
> enable other optimizations (inst combine, inst selection etc).
>
> David
>
> On Thu, Aug 4, 2016 at 8:44 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
> > If you go that route, we should definitely be using a different algorithm
> > and approach here.
> >
> > AST is not likely something you want to use over the entire function(it
> > doesn't scale to either a large number of blocks or a large number of
> > instructions per block.   It's literally N^2.   You also can't sanely add
> > limits in a way that does just turn off the optimization, instead of say,
> > degrading gracefully in how it loses precision), and we both know better
> > general sinking algorithms (including partial dead store elimination,
> etc).
> >
> > I would actually suggest keeping this loop sinking for now, and then
> > designing general sinking from scratch when we want it.
> >
> >
> > On Thu, Aug 4, 2016 at 7:40 PM, Xinliang David Li via llvm-commits
> > <llvm-commits at lists.llvm.org> wrote:
> >>
> >> On Thu, Aug 4, 2016 at 4:53 PM, Chandler Carruth <chandlerc at gmail.com>
> >> wrote:
> >> > (sorry for not replying immediately)
> >> >
> >> > On Sun, Jul 31, 2016 at 7:31 AM Dehao Chen via llvm-commits
> >> > <llvm-commits at lists.llvm.org> wrote:
> >> >>
> >> >> Sorry I didn't update the original thread before I implement this
> >> >> patch.
> >> >> My understanding was Hal proposed IR level sinking at CGP, and I
> >> >> proposed
> >> >> enhancing MachineSinking and Hal thinks that's also an workable plan.
> >> >>
> >> >> But when I started implementing this in MachineSinking, I found that
> >> >> the
> >> >> alias info support for loop at machine level is too weak. That's why
> >> >> MachineLICM only handles function invariant load motion instead of
> loop
> >> >> invariant load motion. I guess that's also the reasoning of the
> >> >> original
> >> >> comment in MachineSink.cpp (Machine sinking should not replace IR
> >> >> sinking).
> >> >
> >> >
> >> > Ah, thanks. This is a great bit of context. It'd be good to mention it
> >> > in
> >> > the patch description
> >> >
> >> >>
> >> >>
> >> >> For now, the design is:
> >> >>
> >> >> 1. IR LICM handles most accurate hoisting at early stage
> >> >> 2. IR Sinking handles most accurate sinking before CGP
> >> >> 3. Machine LICM handles simple hoisting at machine level
> >> >> 4. MachineSinking handles simple sinking at machine level
> >> >>
> >> >> I think this design makes sense because:
> >> >>
> >> >> 1. We don't need to introduce sophisticated alias analysis to machine
> >> >> level loops
> >> >> 2. It's in perfect symmetry
> >> >>
> >> >> But I'm open to discussions on other design alternatives.
> >> >
> >> >
> >> > This design looks great to me, especially considering the interesting
> >> > discovery you made regarding alias info. Of course, it be good to make
> >> > sure
> >> > it mkaes sense Hal as well since he was in the original discussion,
> but
> >> > I
> >> > can't imagine he'd have big objections....
> >> >
> >> > Two things that jump out at me at a high level:
> >> >
> >> > - Should this be a fully general IR sinking pass rather than just a
> >> > *loop*
> >> > sinking pass? I think it'd be fine even if all the non-loop logic was
> >> > just
> >> > documented as an area for future work. If it makes sense in general,
> you
> >> > could leave the framework in place for others to flesh out as test
> cases
> >> > and
> >> > such come up.
> >>
> >> I forgot to reply to this. I think longer term, this should become a
> >> general IR level sinking pass to probably replace IR sinking (cmp
> >> sinking, address computation sinking etc) done in CGP as well.
> >>
> >> David
> >> >
> >> > - It might be worth adding comments to this and the other passes you
> >> > mention
> >> > that document this nice symmetric design and the rationale. Your
> >> > explanation
> >> > here is really great, I just want to make sure others can find it
> later!
> >> > =D
> >> >
> >> > -Chandler
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160805/70a63bc9/attachment.html>