<div dir="ltr">Trying again with the right cfe-dev list address.<div><br></div><div>- Lang. <br><div class="gmail_extra"><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Yep - you're both right. Sorry - it has been too long since I've looked at this.<div><br></div><div>So clang is emitting memcpys for aggregate copies instead of memmoves, even though the pointers might be equal. <span style="line-height:normal">CC'ing cfe-dev, in case anyone there has any insights on why this is.</span></div><div><br></div><div>Digging back through the clang logs it looks like this was a conscious decision, but it's not clear why it worked out this way:</div><div><br></div><div><div>------------------------------------------------------------------------</div><div>r51566 | efriedma | 2008-05-26 05:59:39 -0700 (Mon, 26 May 2008) | 15 lines</div><div><br></div><div>Emit memmove, not memcpy, for structure copies; this is unfortunately </div><div>required for correctness in cases of copying a struct to itself or to </div><div>an overlapping struct (itself for cases like *a = *a, and overlapping </div><div>is possible with unions).</div><div><br></div><div>Hopefully, this won't end up being a perf issue; LLVM *should* be able </div><div>to optimize memmove to memcpy in a lot of cases, and for small copies </div><div>the generated code *should* be mostly comparable. (In reality, LLVM </div><div>is currently horrible at optimizing memmove, but that's a bug, not a </div><div>fundamental issue.)</div><div><br></div><div>gcc currently generates wrong code; that's </div><div><a href="http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32667" target="_blank">http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32667</a>.</div></div><div><br></div><div>...</div><div><br></div><div><div>------------------------------------------------------------------------</div><div>r65699 | lattner | 2009-02-28 10:18:58 -0800 (Sat, 28 Feb 2009) | 5 lines</div><div><br></div><div>after going around in circles a few times, finally cave and emit structure</div><div>copies with memcpy instead of memmove.  This matches what GCC does and if it</div><div>causes a problem with a particular libc we can always fix it with a target</div><div>hook.</div></div><span class="HOEnZb"><font color="#888888"><div><br></div><div><br></div><div>- Lang.</div></font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 18, 2015 at 2:20 PM, 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>----- Original Message -----<br>
> From: "Lang Hames" <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>><br>
> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br>
> Cc: "Commit Messages and Patches for LLVM" <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>>, "Chandler Carruth" <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>><br>
> Sent: Tuesday, August 18, 2015 1:22:14 PM<br>
> Subject: Re: [PATCH] Gut InstCombiner::SimplifyMemTransfer.<br>
><br>
><br>
</span><span>> Hi David, Hal,<br>
><br>
><br>
> Thanks very much for the review!<br>
><br>
><br>
> The clang issue should be fixed, but I think it's orthogonal to this<br>
> patch: Since the memcpys that clang emits aren't annotated they'll<br>
> be treated conservatively by my patch (i.e. not optimized).<br>
><br>
<br>
</span>I don't understand. The logic in InstCombine applies to all memcpys that are expended into individual loads and stores (a size of 1, 2, 4, or 8 bytes and properly aligned), and so you would end up annotating the resulting memory accesses, no?<br>
<span><font color="#888888"><br>
 -Hal<br>
</font></span><div><div><br>
><br>
> Unless I've misunderstood that, I think it should be safe to land<br>
> this once I make the auto* fix that David suggested?<br>
><br>
><br>
> Cheers,<br>
> Lang.<br>
><br>
><br>
><br>
><br>
> On Sun, Aug 16, 2015 at 3:23 AM, Hal Finkel < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> ><br>
> wrote:<br>
><br>
><br>
> ----- Original Message -----<br>
> > From: "Lang Hames via llvm-commits" < <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a> ><br>
> > To: "Chandler Carruth" < <a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a> ><br>
> > Cc: "Commit Messages and Patches for LLVM" <<br>
> > <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a> ><br>
> > Sent: Wednesday, August 12, 2015 9:06:52 PM<br>
> > Subject: Re: [PATCH] Gut InstCombiner::SimplifyMemTransfer.<br>
> ><br>
> ><br>
> ><br>
> > 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<br>
> Clang before we can make this change. Let's add this under a flag<br>
> for testing.<br>
><br>
> -Hal<br>
><br>
><br>
><br>
> ><br>
> ><br>
> > - Lang.<br>
> ><br>
> ><br>
> > On Tue, Jun 30, 2015 at 2:16 PM, Lang Hames < <a href="mailto:lhames@gmail.com" target="_blank">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" target="_blank">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" target="_blank">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.<br>
> > 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 <<br>
> > <a href="mailto:dberlin@dberlin.org" target="_blank">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" target="_blank">chandlerc@gmail.com</a> > wrote:<br>
> > > On Wed, Apr 8, 2015 at 10:15 PM Lang Hames < <a href="mailto:lhames@gmail.com" target="_blank">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<br>
> > >> 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<br>
> > >> 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<br>
> > >> 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,<br>
> > 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>
><br>
><br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">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>
><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>
</div></div></blockquote></div><br></div></div></div>