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

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added a comment.

>> 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.

Sounds reasonable, thanks! Please take a look at the simple fix: D119679 <https://reviews.llvm.org/D119679> .



================
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).
----------------
ABataev wrote:
> 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. 
If one insertelement is a part of several build vector sequences, it is enough to calculate the only one shuffle for any of them, isn't it? Or do you mean issue with scheduling of new shuffles?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119536