<div dir="ltr">Hi David, Hal,<div><br></div><div>Thanks very much for the review!</div><div><br></div><div>The clang issue should be fixed, but I think it's orthogonal to this patch: Since the memcpys that clang emits aren't annotated they'll be treated conservatively by my patch (i.e. not optimized).</div><div><br></div><div>Unless I've misunderstood that, I think it should be safe to land this once I make the auto* fix that David suggested?</div><div><br></div><div>Cheers,</div><div>Lang.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Aug 16, 2015 at 3:23 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"><span class="">----- Original Message -----<br>
> From: "Lang Hames via llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>><br>
> To: "Chandler Carruth" <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>><br>
> Cc: "Commit Messages and Patches for LLVM" <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>><br>
</span><span class="">> Sent: Wednesday, August 12, 2015 9:06:52 PM<br>
> Subject: Re: [PATCH] Gut InstCombiner::SimplifyMemTransfer.<br>
><br>
><br>
><br>
</span>> Another belated *ping*.<br>
><br>
<br>
The SDAG change LGTM.<br>
<br>
The InstCombine change also LGTM, but it seems that we need to fix Clang before we can make this change. Let's add this under a flag for testing.<br>
<span class="HOEnZb"><font color="#888888"><br>
 -Hal<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> - Lang.<br>
><br>
><br>
> On Tue, Jun 30, 2015 at 2:16 PM, Lang Hames < <a href="mailto:lhames@gmail.com">lhames@gmail.com</a> ><br>
> wrote:<br>
><br>
><br>
><br>
> Belated ping. :)<br>
><br>
><br>
> - Lang.<br>
><br>
> On Tue, May 12, 2015 at 2:11 PM, Lang Hames < <a href="mailto:lhames@gmail.com">lhames@gmail.com</a> ><br>
> wrote:<br>
><br>
><br>
><br>
> Gentle ping.<br>
><br>
><br>
> - Lang.<br>
><br>
><br>
><br>
><br>
> On Tue, Apr 28, 2015 at 11:51 AM, Lang Hames < <a href="mailto:lhames@gmail.com">lhames@gmail.com</a> ><br>
> wrote:<br>
><br>
><br>
><br>
> Ping / delayed reply.<br>
><br>
><br>
> DannyB - thanks for the extra context. :)<br>
><br>
><br>
> Chandler - any thoughts? Assuming you still want to keep this<br>
> transform, what do you think of my instcombine change? (i.e. tagging<br>
> the load/store pointers as non-aliasing for memcpy).<br>
><br>
><br>
> Cheers,<br>
> Lang.<br>
><br>
><br>
><br>
><br>
><br>
><br>
> On Tue, Apr 21, 2015 at 9:21 PM, Daniel Berlin < <a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a><br>
> > wrote:<br>
><br>
><br>
> On Wed, Apr 8, 2015 at 10:38 PM, Chandler Carruth <<br>
> <a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a> > wrote:<br>
> > On Wed, Apr 8, 2015 at 10:15 PM Lang Hames < <a href="mailto:lhames@gmail.com">lhames@gmail.com</a> ><br>
> > wrote:<br>
> >><br>
> >> Hi David, Chandler,<br>
> >><br>
> >> The attached patch guts the SimplifyMemTransfer method, which<br>
> >> turns small<br>
> >> memcpys (1/2/4/8 bytes) into load/store pairs. Turning memcpys<br>
> >> into<br>
> >> load/store pairs loses information, since we can no longer assume<br>
> >> the source<br>
> >> and dest are non-overlapping. This is leading to some suboptimal<br>
> >> expansions<br>
> >> for small memcpys on AArch64 when -mno-unaligned-access is turned<br>
> >> on (see<br>
> >> r234462). I suspect other architectures would suffer similar<br>
> >> issues.<br>
> >><br>
> >> I assume this transform is an old workaround to simplify other<br>
> >> non-memcpy-aware IR transforms. These days I think most IR<br>
> >> transforms can<br>
> >> reason sensibly about memcpys, so I'm hoping this is safe to<br>
> >> remove. FWIW,<br>
> >> removing it didn't hit any regression tests except those that were<br>
> >> verifying<br>
> >> that this optimisation was being applied, but then you wouldn't<br>
> >> really<br>
> >> expect it to hit any others.<br>
> ><br>
> ><br>
> > Heh. I tried to remove it before and it regressed a *lot* of<br>
> > performance.<br>
> > Have you measured it? I think there are many places that don't<br>
> > today reason<br>
> > about memcpy but do reason about loads and stores. Here is a<br>
> > partial list:<br>
> ><br>
> > - GVN<br>
><br>
> Note:<br>
><br>
> It reasons about memory intrinsics, including memcpy and memset, and<br>
> will pull values out to forward.<br>
><br>
> for memset, if the load is in the memset size, it will compute what<br>
> the value is.<br>
> For memcpy, it handles anything that is constant memory.<br>
><br>
><br>
> (Certainly, loads and stores are handled better, but ...)<br>
><br>
><br>
><br>
><br>
><br>
><br>
</div></div><div class="HOEnZb"><div class="h5">> _______________________________________________<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/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div></div></blockquote></div><br></div>