[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#1297082, @hiraditya wrote:

> In https://reviews.llvm.org/D35035#1284164, @jfb wrote:
>
> > In https://reviews.llvm.org/D35035#1282236, @spatel wrote:
> >
> > > In https://reviews.llvm.org/D35035#1281462, @lebedev.ri wrote:
> > >
> > > > Not sure what is the general consensus wrt this patch, but i guess it now consistently uses bytes.
> > >
> > >
> > > Agree - the code change looks like what I expected now.
> > >
> > > So:
> > >
> > > 1. Is @jfb objecting to this as an intermediate improvement?
> >
> >
> > My concern is here: https://reviews.llvm.org/D35035#inline-464768
> >  This uses an unrelated constant to drive optimization decisions. Create a new per-target constant.
>
>
> I think the intent of this patch is to do best-effort without relying a lot on target specific constants. Will it help to have per-target constant only for mem* functions? Could that constant be used to drive other optimizations?


Yes, please add a constant which informs this optimization. Don't reuse an unrelated constant for this purpose. Don't use this new constant to dive other unrelated optimizations.

>>> 2. If not, there are unanswered comments about the test diffs.
>>> 3. There was also an unanswered request for the targets and size/perf improvements for this change (presumably some 32-bit target shows wins).
>>> 4. Are there any updates on the perf for the ideal change (removing this expansion completely)?
>> 
>> I do want to see size and perf results.


Repository:
  rL LLVM

https://reviews.llvm.org/D35035