<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr 16, 2015 at 8:11 AM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><br><br><hr><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><span class=""><b>From: </b>"Xinliang David Li" <<a href="mailto:xinliangli@gmail.com" target="_blank">xinliangli@gmail.com</a>><br></span><b>To: </b>"Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br><b>Cc: </b>"Commit Messages and Patches for LLVM" <<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>>, "reviews+d8911+public+8f2845face98911b" <<a href="mailto:reviews%2BD8911%2Bpublic%2B8f2845face98911b@reviews.llvm.org" target="_blank">reviews+D8911+public+8f2845face98911b@reviews.llvm.org</a>>, "Nick Lewycky" <<a href="mailto:nicholas@mxc.ca" target="_blank">nicholas@mxc.ca</a>><br><b>Sent: </b>Wednesday, April 15, 2015 11:36:58 PM<span class=""><br><b>Subject: </b>Re: [PATCH] Fix a performance problem in gep(gep ...) merging<br><br></span><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Tue, Apr 14, 2015 at 10:58 AM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br></span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span><hr><span class=""><br>
> From: "Xinliang David Li" <<a href="mailto:xinliangli@gmail.com" target="_blank">xinliangli@gmail.com</a>><br>
> To: "Nick Lewycky" <<a href="mailto:nicholas@mxc.ca" target="_blank">nicholas@mxc.ca</a>><br>
> Cc: "Commit Messages and Patches for LLVM" <<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>>,<br>
> <a href="mailto:reviews%2BD8911%2Bpublic%2B8f2845face98911b@reviews.llvm.org" target="_blank">reviews+D8911+public+8f2845face98911b@reviews.llvm.org</a><br>
> Sent: Tuesday, April 14, 2015 10:23:36 AM<br>
> Subject: Re: [PATCH] Fix a performance problem in gep(gep ...) merging<br>
><br>
><br>
><br>
> IIUC, GEP merging can be harmful regardless of where the target is in<br>
> loop context or not. Wei's heuristic filters out lots of harmful<br>
> cases. A more general fix might need introduce more sophisticated<br>
> cost analysis -- it is possible (as mentioned by Quentin) for this<br>
> simple heuristic to filter out beneficial merging, but test cases<br>
> are needed for that.<br>
<br>
</span></span><span class="">One thing we need to keep in mind here is that the reduction in GEP depth directly affects the precision of BasicAA (and other code that calls GetUnderlyingObject/GetUnderlyingObjects. These things have small recursion cutoffs (6 currently). computeKnownBits, which looks through GEPs to find pointer alignments is another example. The aggressive merging strategy supports these small cutoffs; if we reduce the amount of merging that we do, we may need to re-tune these cutoffs.<br></span></blockquote><span class=""><div><br></div><div>In practice, how often do we see long chains of GEPs? 6 seems reasonably long. The alias analysis does not really depend on the merged form of GEP to be more precise (other than the arbitrary limitation imposed on long GEP chains).</div></span></div></div></div></blockquote><br>I suspect not often, but I don't really know. It might be a good idea to add a statistic to count how often we hit the limit. To be clear, I'm not against this change, but I wanted people to keep these recursion depth limits in mind as it might be relevant to adjusting for any regressions should they emerge.<br></div></div></blockquote><div><br></div><div>Agreed. Also useful to do this with GEP merge disabled.</div><div> </div><div>thanks,</div><div><br></div><div>David</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><br>Interesting example.<br><br>Thanks again,<br>Hal<div><div class="h5"><br><br><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><br></div><div>In fact, there are cases where merged GEP actually confuses aliaser. Consider the following case:</div><div><br></div><div><div>extern int aa[];</div><div><br></div><div>int foo(int k, int n</div><div>#ifdef NOMERGE</div><div>        , int *bb</div><div>#endif</div><div>        )</div><div>{</div><div>   int i, s;</div><div>   s = 0;</div><div>#ifndef NOMERGE</div><div>   int *bb = &aa[k];</div><div>#endif</div><div>   s = bb[n];</div><div>   bb[n+1] = 20;</div><div>   s+=bb[n];</div><div><br></div><div>   return s;</div><div>}</div><div><br></div></div><div>When built with -DNOMERGE or without but with instcombine disabled, GVN is able to remove the second load. However, with instcombine turned on and GEP merged, the load remains after the store.</div><div><br></div><div>David</div><div><br></div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
 -Hal<br>
<div><div><br>
><br>
><br>
> David<br>
><br>
><br>
> On Tue, Apr 14, 2015 at 1:16 AM, Nick Lewycky < <a href="mailto:nicholas@mxc.ca" target="_blank">nicholas@mxc.ca</a> ><br>
> wrote:<br>
><br>
><br>
> Quentin Colombet wrote:<br>
><br>
><br>
> The thing with the "both constants" approach is that we may miss some<br>
> cases where an addressing mode may apply. In particular, on x86,<br>
> when one argument of the add is a constant, we would be able to<br>
> match reg1 + reg2 + displacement.<br>
><br>
> That being said, your numbers imply this is not a problem in<br>
> practice, and most targets do not support reg1 + reg2 + reg3/imm<br>
> addressing mode. Anyway, we could recover from that later on if<br>
> needed.<br>
><br>
> So LGTM.<br>
><br>
> I might be misunderstanding why this patch helps, please bear with<br>
> me. Is this not a special case of a general problem, mainly that<br>
> we're merging something into a loop that was previously two<br>
> operations one inside and one outside the loop? Why wouldn't this<br>
> problem come up in other ways? We already have other problems where<br>
> we introduce loop carried dependencies where ones previously didn't<br>
> exist, shouldn't we approach this by adding an API to detect those<br>
> and then making transforms conditional on that?<br>
><br>
> Nick<br>
><br>
><br>
><br>
><br>
><br>
><br>
> Thanks Wei!<br>
> -Quentin<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/" target="_blank">http://reviews.llvm.org/</a> settings/panel/ emailpreferences/<br>
><br>
><br>
><br>
><br>
> ______________________________ _________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/" target="_blank">http://lists.cs.uiuc.edu/</a> mailman/listinfo/llvm-commits<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">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>
</div></div><span><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div></div>
</blockquote><br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></div></div></div></div></blockquote></div><br></div></div>