[PATCH] D119536: [SLP] Extract intermediate insertelement for external use

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

In D119536#3316636 <https://reviews.llvm.org/D119536#3316636>, @anton-afanasyev wrote:

> In D119536#3314640 <https://reviews.llvm.org/D119536#3314640>, @ABataev wrote:
>
>> Hmm, looks like the problem appears only if we have insertelements to be vectorized not from `vectorizeInsertElementInst`, where we have a proper check. Maybe just add a check for insertelements in other functions and just expect them from `vectorizeInsertElementInst`?
>
> Well, that would definitely solve the problem. There is also another way: on the contrary, remove `hasOneUse()` check from `vectorizeInsertElementInst`. This patch allows it.

Yes, sure, but it will require some extra stuff and some time to polish and fix all corner cases. If we’d like to fix it in 14.0, better to start with a simple fix and then try to implement more complex feature.



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5690-5711
     if (!isa_and_nonnull<InsertElementInst>(EU.User) &&
         !ExtractCostCalculated.insert(EU.Scalar).second)
       continue;
 
     // Uses by ephemeral values are free (because the ephemeral value will be
     // removed prior to code generation, and so the extraction will be
     // removed as well).
----------------
anton-afanasyev wrote:
> ABataev wrote:
> > What if the same insertelement is used in several users? Would not we miss some cost here?
> Why? See line 5924 above, "We only add extract cost once for the same scalar", comment states.
Currently it is not quite correct for scalars (too optimistic) but will be fixed in future, once improved reductions support is committed. But for insertelements it is not correct for sure. If one instruction is a part of several build vector sequences, you need to calculate the shuffles for each of them independently. Same applies to the final vector emission. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119536/new/

https://reviews.llvm.org/D119536