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

Fri Aug 5 10:37:14 PDT 2016

On Fri, Aug 5, 2016 at 8:20 AM, Daniel Berlin <dberlin at dberlin.org> wrote:
>
> 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

Ah -- for some reason, I did not realize you were talking about the
n^2 behavior of alias set tracker.  I somehow misread 'AST' :)

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

I checked the code -- it is indeed N^2 -- the culprit is in
mergeAliasSetsForPointer -- for each newly added 'pointer' deref, it
walks through all existing alias sets (including those merged with
forward field set). It can be really bad when all alias sets are
'disjoint'.

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

Yes, totally agree. To be clear what I proposed for the future is a
centralized pass to do sinking like transformations with no
implication on implementation detail.  AST is in its current form will
be bad to used at function level.

thanks,

David

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