[PATCH] D83779: [SLP] Fix order of `insertelement`/`insertvalue` seed operands

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7091
+    AggregateSize = IE->getType()->getNumElements();
+    return true;
+  }
----------------
returns `true` for the function, that shall return `unsigned`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7099
+    if (auto *ST = dyn_cast<StructType>(CurrentType)) {
+      for (auto Elt : ST->elements())
+        if (Elt != ST->getElementType(0)) // check homogeneity
----------------
`Type *`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7123
+    if (auto *CI = dyn_cast<ConstantInt>(IE->getOperand(2))) {
+      auto *VT = dyn_cast<VectorType>(IE->getType());
+      OperandIndex *= VT->getNumElements();
----------------
Just `cast`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7140
+      CurrentType = AT->getElementType();
+    } else {
+      return false;
----------------
What about VectorType here if it is allowed in the previous function?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7160
+        isa<InsertValueInst>(InsertedOperand)) {
+      if (!findBuildAggregate_rec(dyn_cast<Instruction>(InsertedOperand), TTI,
+                                  BuildVectorOpds, InsertElts, OperandIndex))
----------------
`cast<Instruction>`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7163-7166
+    } else {
+      BuildVectorOpds[OperandIndex] = InsertedOperand;
+      InsertElts[OperandIndex] = LastInsertInst;
+    }
----------------
No need for `else` here


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7170-7173
+    if (LastInsertInst == nullptr ||
+        (!isa<InsertValueInst>(LastInsertInst) &&
+         !isa<InsertElementInst>(LastInsertInst)) ||
+        !LastInsertInst->hasOneUse())
----------------
Better to transform it a condition of the loop and return `false` after the end of the loop


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7214-7222
+    BuildVectorOpds.erase(
+        std::remove_if(BuildVectorOpds.begin(), BuildVectorOpds.end(),
+                       [](const Value *V) { return V == nullptr; }),
+        BuildVectorOpds.end());
+
+    InsertElts.erase(
+        std::remove_if(InsertElts.begin(), InsertElts.end(),
----------------
Just `llvm::erase_value(<container>, nullptr)`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7225-7226
+    if (BuildVectorOpds.size() >= 2)
+      return true;
+  }
+
----------------
Tabs


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7326
   if (!findBuildAggregate(IEI, TTI, BuildVectorOpds, BuildVectorInsts) ||
-      BuildVectorOpds.size() < 2 ||
       (llvm::all_of(BuildVectorOpds,
----------------
Why you don't need this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83779