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

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added a comment.

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.


Makes sense.

> 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.
> 
> If that doesn't work though, using LargestLegalIntTypeSizeInBits still seems like a good compromise to me. We want to conservatively limit the expansion to a size/type that the target tells us is ok (can be performed with a single load/store), and that's the value that most closely matches what we have today, so we avoid regressions as we work to the goal. It's not the ideal change, but there's precedence for this sort of datalayout use in instcombine (see InstCombiner::shouldChangeType()). Adding a new specifier to the datalayout to account for things like pair ops or vectors doesn't make sense to me - that moves us away from the goal of improving the other passes and removing the expansion in instcombine.

Do you think getting this patch in is a good for now. After some performance analysis if we find that we dont need to inline memcpy here then we can remove this entirely at a later stage.


https://reviews.llvm.org/D35035