[PATCH] D35035: [InstCombine] Prevent memcpy generation for small data size

[JF Bastien via Phabricator via llvm-commits]

jfb added a comment.

In https://reviews.llvm.org/D35035#1253023, @spatel wrote:

> In https://reviews.llvm.org/D35035#1252976, @lebedev.ri wrote:
>
> > In https://reviews.llvm.org/D35035#1252975, @spatel wrote:
> >
> > > In https://reviews.llvm.org/D35035#1252829, @hiraditya wrote:
> > >
> > > > In https://reviews.llvm.org/D35035#1252796, @SjoerdMeijer wrote:
> > > >
> > > > > Perhaps the impact is negligible, non-existent, and we worry about this for nothing. As also suggested earlier, I will try to get some numbers on the table for ARM and AArch64 if we strip out the lowering here, if that is helpful for this discussion, but probably need a day or two to get them.
> > > >
> > > >
> > > > If you could provide some numbers, I can go ahead and remove the inlining of memcpy altogether provided the reviewers agree with it, or we can merge this patch which is trying to improve on previously hardcoded numbers.
> > >
> > >
> > > Yes, I support removing the expansion entirely, but I don't think we can commit that change without doing some advance perf testing.
> > >  And yes, in the best case, we'll discover that there are no regressions because all of the other analyses and lowering will do the transform as intended when it's profitable.
> >
> >
> > Just to drop a nit, here is a somewhat idiomatic pattern, that is recommended to avoid breaking strict aliasing: https://godbolt.org/z/_JNwOp
> >  If we don't do this `memcpy` expansion where, will we have `memcpy` till the backend?
> >  Surely this is not good.
>
>
> That case (and a couple of similar tests that I tried) are handled by -sroa, so they probably never made it to instcombine in the 1st place. I don't know anything about SROA, but hopefully, it's making that transform using some principled logic. :)


clang also performs some of this work, even at O0.


https://reviews.llvm.org/D35035