[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:2840-2845
+      int MinIndex = INT_MAX;
+      for (auto *V : VL)
+        MinIndex = std::min(
+            MinIndex,
+            int(cast<ConstantInt>(cast<InsertElementInst>(V)->getOperand(2))
+                    ->getZExtValue()));
----------------
Outline this code into a separate helper function


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2846-2853
+      bool IsConsecutive = true;
+      for (int I = 0, E = VL.size(); I < E; I++) {
+        int Index =
+            cast<ConstantInt>(cast<InsertElementInst>(VL[I])->getOperand(2))
+                ->getZExtValue() -
+            MinIndex;
+        IsConsecutive &= (I == Index);
----------------
Beter to turn this into lambda


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3505
     ScalarTy = CI->getOperand(0)->getType();
+  else if (InsertElementInst *IE = dyn_cast<InsertElementInst>(VL[0]))
+    ScalarTy = IE->getOperand(1)->getType();
----------------
`auto *IE`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3639
+      InstructionCost Cost = 0;
+      auto *SrcVecTy = cast<FixedVectorType>(VL[0]->getType());
+
----------------
`VL[0]`->`VL0`?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3650-3656
+      // TODO: fix getShuffleCost() for SK_InsertSubvector
+      // and uncomment this to accurately tune costs. For typical cases
+      // this should be zero when subvector is whole register.
+      // if (SrcVecTy->getNumElements() != VL.size()) {
+      //   Cost += TTI->getShuffleCost(TargetTransformInfo::SK_InsertSubvector,
+      //                               SrcVecTy, None, MinIndex, VecTy);
+      // }
----------------
I think this should be uncommented, otherwise, we are too optimistic about the cost.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3975
+  // Vectorizing inserts of gathered values is redundant.
+  if (isa<InsertElementInst>(VectorizableTree[0]->Scalars[0]) &&
+      VectorizableTree[1]->State == TreeEntry::NeedToGather)
----------------
I think better to check that all elements are `InsertElementInst` and move this check closer  to the end of the function


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4204
+    if (auto *Insert = dyn_cast<InsertElementInst>(EU.Scalar)) {
+      auto &Scalars = getTreeEntry(EU.Scalar)->Scalars;
+      SmallVector<int> Mask(Scalars.size(), -1);
----------------
Better to create a set here


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4205
+      auto &Scalars = getTreeEntry(EU.Scalar)->Scalars;
+      SmallVector<int> Mask(Scalars.size(), -1);
+      int MinIndex = INT_MAX;
----------------
Use `UndefMaskElem` instead of `-1`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4227-4228
+        auto *VecTy = cast<FixedVectorType>(EU.Scalar->getType());
+        ExtractCost += TTI->getShuffleCost(
+            TargetTransformInfo::SK_PermuteSingleSrc, VecTy, Mask);
+      }
----------------
Check for the kind of permutation in `Mask`, it may be an Identity of a Reverse kind.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4575
     ScalarTy = Store->getValueOperand()->getType();
+  else if (InsertElementInst *IE = dyn_cast<InsertElementInst>(VL0))
+    ScalarTy = IE->getOperand(1)->getType();
----------------
`auto *IE`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4636-4678
+    case Instruction::InsertElement: {
+      Builder.SetInsertPoint(VL0);
+      Value *V = vectorizeTree(E->getOperand(0));
+
+      const int NumElts =
+          cast<FixedVectorType>(VL0->getType())->getNumElements();
+      const int NumScalars = E->Scalars.size();
----------------
Can we leave it to InstCombiner and return the original insrtelement instruction as a result?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5163
+      // Generate extract shuffle when Scalar is not the last insert
+      SmallVector<int> Mask(E->Scalars.size(), -1);
+      int MinIndex = INT_MAX;
----------------
`UndefMaskElem`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5189-5190
+
+      LLVM_DEBUG(dbgs() << "SLP: Replaced:" << *User << ".\n");
+    } else if (auto *VecI = dyn_cast<Instruction>(Vec)) {
       if (PHINode *PH = dyn_cast<PHINode>(User)) {
----------------
```
  continue;
}
if (...) {
```


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6525-6527
+      ArrayRef<Value *> Ops = InsertUses.empty()
+                                  ? VL.slice(I, OpsWidth)
+                                  : InsertUses.slice(I, OpsWidth);
----------------
Why not just pass `InsertUses` as ёМДё here and drop `InsertUses` parameter completely?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98714