[PATCH] D98714: [SLP] Add insertelement instructions to vectorizable tree

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev marked 4 inline comments as done.
anton-afanasyev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4670-4671
+        // Create shuffle to resize vector
+        for (unsigned I = 0; I < NumElts; I++)
+          Mask[I] = (I < NumScalars) ? I : UndefMaskElem;
+        V = Builder.CreateShuffleVector(V, UndefValue::get(V->getType()), Mask);
----------------
ABataev wrote:
> ```
> SmallVector<int, 16> Mask(NumElts, UndefMaskElem);
> std::iota(Mask.begin(), std::next(Mask.begin(), NumScalars), 0); 
> ```
> 
> Also, I think just `std::iota(Mask.begin(), Mask.end(), 0); ` shall work too
Thanks, changed to `std::iota(Mask.begin(), std::next(Mask.begin(), NumScalars), 0);`

Yes, `std::iota(Mask.begin(), Mask.end(), 0);` gives the same result, but wouldn't it lead to redundant code lowered?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4672-4680
+        V = Builder.CreateShuffleVector(V, UndefValue::get(V->getType()), Mask);
+
+        for (unsigned I = 0; I < NumElts; I++)
+          Mask[I] =
+              (I < MinIndex || I >= MaxIndex) ? I : NumElts - MinIndex + I;
+
+        V = Builder.CreateShuffleVector(
----------------
ABataev wrote:
> ABataev wrote:
> > Can we use `ShuffleBuilder` here?
> Did you include these costs in the cost model?
They are included in `getScalarizationOverhead()`.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4672-4680
+        V = Builder.CreateShuffleVector(V, UndefValue::get(V->getType()), Mask);
+
+        for (unsigned I = 0; I < NumElts; I++)
+          Mask[I] =
+              (I < MinIndex || I >= MaxIndex) ? I : NumElts - MinIndex + I;
+
+        V = Builder.CreateShuffleVector(
----------------
anton-afanasyev wrote:
> ABataev wrote:
> > ABataev wrote:
> > > Can we use `ShuffleBuilder` here?
> > Did you include these costs in the cost model?
> They are included in `getScalarizationOverhead()`.
Yes, I thought about `ShuffleBuilder`, but here we just need to create two shuffles of special kind for vector resizing. It requires `ShuffleInstructionBuilder` expanding, don't think it's worth it.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5183
+        NumInsertsUsed++;
+      } while (is_contained(Scalars, Insert));
+
----------------
ABataev wrote:
> `Scalars.contain(Insert)`
Thanks, done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98714