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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2871-2881
+      unsigned MinIndex = INT_MAX;
+      for (Value *V : VL)
+        MinIndex = std::min(MinIndex, *getInsertIndex(V, 0));
+
+      bool IsConsecutive = true;
+      for (unsigned I = 0, E = VL.size(); I < E; I++)
+        IsConsecutive &= (I == *getInsertIndex(VL[I], 0) - MinIndex);
----------------
I think this can be simplified:
```
int Offset = *getInsertIndex(VL[0], 0);
ValueList Operands(VL.size());
Operands[0] = cast<Instruction>(VL[0])->getOperand(1);
bool IsConsecutive = true;
for (unsigned I = 1, E = VL.size(); I < E; ++I) {
  IsConsecutive &= (I == *getInsertIndex(VL[I], 0) - Offset);
  Operands[I] = cast<Instruction>(VL[I])->getOperand(1);
}
```


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2883
+
+      if (IsConsecutive) {
+        TreeEntry *TE = newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx);
----------------
If not consecutive, we can include the cost of single permute and still vectorize it, no?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3666-3667
+      APInt DemandedElts = APInt::getNullValue(NumElts);
+      for (auto *V : VL)
+        DemandedElts.setBit(*getInsertIndex(V, 0));
+
----------------
Hm, I think you need to normalize insert indices here, the `MinIndex` (offset) might be non `0` but should be 0. Plus, I think, need to add (subtract from vectcost) a cost of subvector insertion from Index `MinIndex` (if `MinIndex != 0`)


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4236-4240
+      if (NumInsertsUsed < Scalars.size()) {
+        auto *VecTy = cast<FixedVectorType>(EU.Scalar->getType());
+        ExtractCost += TTI->getShuffleCost(
+            TargetTransformInfo::SK_PermuteSingleSrc, VecTy, Mask);
+      }
----------------
I think this should be:
```
ExtractCost += TTI->getShuffleCost(TTI::SK_InsertSubvector, VL0->getType(), MinIndex, VecTy);
```


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5166
+    if (auto *Insert = dyn_cast<InsertElementInst>(Scalar)) {
+      Builder.SetInsertPoint(cast<Instruction>(User));
+      Value *V = Vec;
----------------
What if the user is Phi in thу middle of other phis?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5172-5174
+      unsigned MinIndex = INT_MAX;
+      for (auto *S : Scalars)
+        MinIndex = std::min(MinIndex, *getInsertIndex(S, 0));
----------------
You're using this code in many places, make it a function.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5184-5185
+
+      if (NumInsertsUsed != Scalars.size())
+        V = Builder.CreateShuffleVector(V, UndefValue::get(V->getType()), Mask);
+
----------------
Why we can't use just `V` here? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98714