<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Apr 15, 2015 at 1:16 PM, Wei Mi <span dir="ltr"><<a href="mailto:wmi@google.com" target="_blank">wmi@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Great, Thanks Mark! I will do the performance for it on x86.<br></blockquote><div><br></div><div>Thanks!  Like I said there may still be some bugs to work out with the patch as is.  I haven't yet run much code through it.</div><div><br></div><div>Mark</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
On Wed, Apr 15, 2015 at 12:52 PM, Mark Heffernan <<a href="mailto:meheff@google.com">meheff@google.com</a>> wrote:<br>
> On Wed, Apr 15, 2015 at 12:16 PM, Xinliang David Li <<a href="mailto:xinliangli@gmail.com">xinliangli@gmail.com</a>><br>
> wrote:<br>
>><br>
>> Mark, sounds great. Can you send the patch to Wei for testing ?<br>
><br>
><br>
> Patch is attached.  Not yet ready for review though ;-)<br>
><br>
> For the attached patch I added it to the x86 target.  Currently I don't plan<br>
> to do that when I check in the patch.  It'll just be added to those targets<br>
> which use the existing pass which this transformation is added to.  It can<br>
> be extended to other targets later.  The patch breaks a few x86 tests maybe<br>
> because the codegen pipeline is now different (also has CSE and LICM) or<br>
> maybe because there is a bug in my code.<br>
><br>
> Mark<br>
>><br>
>><br>
>> David<br>
>><br>
>> On Wed, Apr 15, 2015 at 11:34 AM, Mark Heffernan <<a href="mailto:meheff@google.com">meheff@google.com</a>><br>
>> wrote:<br>
>>><br>
>>> There was a discussion on the mailing list about the exact issue this<br>
>>> patch is addressing.  We (Google team working on NVPTX backend) ran into<br>
>>> this same issue as did someone (Francois Pichet) working with the ppc<br>
>>> backend.  Here's the link to the discussion:<br>
>>><br>
>>> <a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-March/083398.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-March/083398.html</a><br>
>>><br>
>>> I initially proposed what your patch does.  That is, don't merge geps<br>
>>> unless the indices are constant (in which case the add is folded so no<br>
>>> additional instructions are potentially created in loops).  However, after<br>
>>> some discussion we came to the conclusion that the gep merging is a good<br>
>>> canonicalization which we want to do.  Instead a better approach might be to<br>
>>> split GEPs (undoing the early GEP merge) very late, right before codegen, if<br>
>>> it is determined to be profitable.  This way, all the intermediate passes<br>
>>> get the advantage of the canonicalization, while still being able to do<br>
>>> LICM/CSE on GEP subexpressions which the gep merge had prevented.<br>
>>><br>
>>> I have a patch I am in the last stages of testing which will perform this<br>
>>> optimization in the pass SeparateConstOffsetFromGEP (to be renamed<br>
>>> ReassociateGEPs... or maybe SplitGEPs now that I think about it).  I ran my<br>
>>> patch with your example from bug 23163 and it looks to perform better than<br>
>>> the prevent gep merging approach does.  With head compiler I get 29<br>
>>> instructions in the inner loop (this matches what you report in the bug).<br>
>>> You mention disabling merging reduces the loop instructions down to 25<br>
>>> instructions.  With the patch I am working on, it gets reduced down to 16<br>
>>> instructions.<br>
>>><br>
>>> The pass is currently enabled only for select backends: nvptx, arm (-O3),<br>
>>> and ppc (-O3).   For testing with your bug I added it in x86 addIRPasses()<br>
>>> followed by CSE and LICM (these extra passes do nothing on your example<br>
>>> without the GEP splitting pass).<br>
>>><br>
>>> I should have the patch out for review later in the week.<br>
>>><br>
>>> Sorry for the duplicate effort here.  I should have filed a bug on this<br>
>>> issue before diving in :-(<br>
>>><br>
>>> Mark<br>
>>><br>
>>><br>
>>> REPOSITORY<br>
>>>   rL LLVM<br>
>>><br>
>>> <a href="http://reviews.llvm.org/D8911" target="_blank">http://reviews.llvm.org/D8911</a><br>
>>><br>
>>> EMAIL PREFERENCES<br>
>>>   <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
>>><br>
>>><br>
>>><br>
>>> _______________________________________________<br>
>>> llvm-commits mailing list<br>
>>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>><br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
</div></div></blockquote></div><br></div></div>