<div dir="ltr">(sorry for not replying immediately)<div><br><div class="gmail_quote"><div dir="ltr">On Sun, Jul 31, 2016 at 7:31 AM Dehao Chen via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.<div><br></div><div>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).</div></div></blockquote><div><br></div><div>Ah, thanks. This is a great bit of context. It'd be good to mention it in the patch description</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>For now, the design is:</div><div><br></div><div>1. IR LICM handles most accurate hoisting at early stage</div><div>2. IR Sinking handles most accurate sinking before CGP</div><div>3. Machine LICM handles simple hoisting at machine level</div><div>4. MachineSinking handles simple sinking at machine level</div><div><br></div><div>I think this design makes sense because:</div><div><br></div><div>1. We don't need to introduce sophisticated alias analysis to machine level loops</div><div>2. It's in perfect symmetry</div><div><br></div><div>But I'm open to discussions on other design alternatives.</div></div></blockquote><div><br></div><div>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....</div><div><br></div><div>Two things that jump out at me at a high level:</div><div><br></div><div>- 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.</div><div><br></div><div>- 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</div><div><br></div><div>-Chandler</div></div></div></div>