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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

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`?



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:722
+  auto *LastInsert = cast<Instruction>(*find_if(Inserts, [Inserts](Value *V) {
+    return llvm::all_of(Inserts, [V](Value *VV) {
+      return V != cast<Instruction>(VV)->getOperand(0);
----------------
Drop `llvm:`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:730
+    SmallVector<int> Mask(NumElts);
+    std::iota(Mask.begin(), std::next(Mask.begin(), NumElts), 0);
+
----------------
`std::next(Mask.begin(), NumElts)` -> `Mask.end()`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:730
+    SmallVector<int> Mask(NumElts);
+    std::iota(Mask.begin(), std::next(Mask.begin(), NumElts), 0);
+
----------------
ABataev wrote:
> `std::next(Mask.begin(), NumElts)` -> `Mask.end()`
Would be good to fill unused elements with UndefMaskElem instead, which may help to improve cost in the future for large vectors.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:734
+    while (CurrentInsert != Insert) {
+      Optional<int> InsertIdx = getInsertIndex(CurrentInsert);
+      Mask[*InsertIdx] = *InsertIdx + NumElts;
----------------
What if `InsertIdx` is `UndefMaskElem`?


================
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).
----------------
What if the same insertelement is used in several users? Would not we miss some cost here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119536