<div dir="ltr"><div><br></div><div>Assume literally all of that is 100% correct, and it should never change.</div><div>The first part of what i said is still true.</div><div><br>The approach of using AliasSetTracker stil makes this something you do not want to run over the entire function.</div><div>In fact, as written, your pass should *already* have the same issue as <a href="https://llvm.org/bugs/show_bug.cgi?id=28832">https://llvm.org/bugs/show_bug.cgi?id=28832</a>, and be slow on the same testcase</div><div><br></div><div>(there is a larger testcase for that where it will take about 600+ seconds, instead of 100+)</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 4, 2016 at 9:39 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This is intended to be an centralized pass (not unified algorithm) to<br>
do a whole bunch of very specialized code sinking, which is different<br>
from anticipation based redundancy (store) elimination, nor is it<br>
limited to stores. In fact, the name 'sinking' is misleading. It is<br>
more a general forward propagation pass (for instructions that have<br>
multiple uses in multiple BBs) to reduce overall frequency or to<br>
enable other optimizations (inst combine, inst selection etc).<br>
<span class=""><font color="#888888"><br>
David<br>
</font></span><div class=""><div class="h5"><br>
On Thu, Aug 4, 2016 at 8:44 PM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br>
> If you go that route, we should definitely be using a different algorithm<br>
> and approach here.<br>
><br>
> AST is not likely something you want to use over the entire function(it<br>
> doesn't scale to either a large number of blocks or a large number of<br>
> instructions per block. It's literally N^2. You also can't sanely add<br>
> limits in a way that does just turn off the optimization, instead of say,<br>
> degrading gracefully in how it loses precision), and we both know better<br>
> general sinking algorithms (including partial dead store elimination, etc).<br>
><br>
> I would actually suggest keeping this loop sinking for now, and then<br>
> designing general sinking from scratch when we want it.<br>
><br>
><br>
> On Thu, Aug 4, 2016 at 7:40 PM, Xinliang David Li via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> On Thu, Aug 4, 2016 at 4:53 PM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>><br>
>> wrote:<br>
>> > (sorry for not replying immediately)<br>
>> ><br>
>> > On Sun, Jul 31, 2016 at 7:31 AM Dehao Chen via llvm-commits<br>
>> > <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> >><br>
>> >> Sorry I didn't update the original thread before I implement this<br>
>> >> patch.<br>
>> >> My understanding was Hal proposed IR level sinking at CGP, and I<br>
>> >> proposed<br>
>> >> enhancing MachineSinking and Hal thinks that's also an workable plan.<br>
>> >><br>
>> >> But when I started implementing this in MachineSinking, I found that<br>
>> >> the<br>
>> >> alias info support for loop at machine level is too weak. That's why<br>
>> >> MachineLICM only handles function invariant load motion instead of loop<br>
>> >> invariant load motion. I guess that's also the reasoning of the<br>
>> >> original<br>
>> >> comment in MachineSink.cpp (Machine sinking should not replace IR<br>
>> >> sinking).<br>
>> ><br>
>> ><br>
>> > Ah, thanks. This is a great bit of context. It'd be good to mention it<br>
>> > in<br>
>> > the patch description<br>
>> ><br>
>> >><br>
>> >><br>
>> >> For now, the design is:<br>
>> >><br>
>> >> 1. IR LICM handles most accurate hoisting at early stage<br>
>> >> 2. IR Sinking handles most accurate sinking before CGP<br>
>> >> 3. Machine LICM handles simple hoisting at machine level<br>
>> >> 4. MachineSinking handles simple sinking at machine level<br>
>> >><br>
>> >> I think this design makes sense because:<br>
>> >><br>
>> >> 1. We don't need to introduce sophisticated alias analysis to machine<br>
>> >> level loops<br>
>> >> 2. It's in perfect symmetry<br>
>> >><br>
>> >> But I'm open to discussions on other design alternatives.<br>
>> ><br>
>> ><br>
>> > This design looks great to me, especially considering the interesting<br>
>> > discovery you made regarding alias info. Of course, it be good to make<br>
>> > sure<br>
>> > it mkaes sense Hal as well since he was in the original discussion, but<br>
>> > I<br>
>> > can't imagine he'd have big objections....<br>
>> ><br>
>> > Two things that jump out at me at a high level:<br>
>> ><br>
>> > - Should this be a fully general IR sinking pass rather than just a<br>
>> > *loop*<br>
>> > sinking pass? I think it'd be fine even if all the non-loop logic was<br>
>> > just<br>
>> > documented as an area for future work. If it makes sense in general, you<br>
>> > could leave the framework in place for others to flesh out as test cases<br>
>> > and<br>
>> > such come up.<br>
>><br>
>> I forgot to reply to this. I think longer term, this should become a<br>
>> general IR level sinking pass to probably replace IR sinking (cmp<br>
>> sinking, address computation sinking etc) done in CGP as well.<br>
>><br>
>> David<br>
>> ><br>
>> > - It might be worth adding comments to this and the other passes you<br>
>> > mention<br>
>> > that document this nice symmetric design and the rationale. Your<br>
>> > explanation<br>
>> > here is really great, I just want to make sure others can find it later!<br>
>> > =D<br>
>> ><br>
>> > -Chandler<br>
>> ______________________________<wbr>_________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>