<div dir="ltr">If you go that route, we should definitely be using a different algorithm and approach here.<div><br></div><div>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).</div><div><br></div><div>I would actually suggest keeping this loop sinking for now, and then designing general sinking from scratch when we want it.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 4, 2016 at 7:40 PM, Xinliang David Li via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Aug 4, 2016 at 4:53 PM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>> 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 patch.<br>
>> My understanding was Hal proposed IR level sinking at CGP, and I 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 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 original<br>
>> comment in MachineSink.cpp (Machine sinking should not replace IR sinking).<br>
><br>
><br>
> Ah, thanks. This is a great bit of context. It'd be good to mention it 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 sure<br>
> it mkaes sense Hal as well since he was in the original discussion, but 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 *loop*<br>
> sinking pass? I think it'd be fine even if all the non-loop logic was 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 and<br>
> such come up.<br>
<br>
</div></div>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>
<span class="im HOEnZb">><br>
> - It might be worth adding comments to this and the other passes you mention<br>
> that document this nice symmetric design and the rationale. Your explanation<br>
> here is really great, I just want to make sure others can find it later! =D<br>
><br>
> -Chandler<br>
</span><div class="HOEnZb"><div class="h5">______________________________<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>
</div></div></blockquote></div><br></div>