<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">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><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">----- Original Message -----<br>
> From: "Xinliang David Li" <<a href="mailto:xinliangli@gmail.com">xinliangli@gmail.com</a>><br>
> To: "Nick Lewycky" <<a href="mailto:nicholas@mxc.ca">nicholas@mxc.ca</a>><br>
> Cc: "Commit Messages and Patches for LLVM" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>>,<br>
> <a href="mailto:reviews%2BD8911%2Bpublic%2B8f2845face98911b@reviews.llvm.org">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>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></blockquote><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><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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
-Hal<br>
<div class=""><div class="h5"><br>
><br>
><br>
> David<br>
><br>
><br>
> On Tue, Apr 14, 2015 at 1:16 AM, Nick Lewycky < <a href="mailto:nicholas@mxc.ca">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">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">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 class=""><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>