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

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev marked 10 inline comments as done.
anton-afanasyev added a comment.

Addressed @ABataev's comments.



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7091
+    AggregateSize = IE->getType()->getNumElements();
+    return true;
+  }
----------------
ABataev wrote:
> returns `true` for the function, that shall return `unsigned`
Thanks, I've forgotten to change return type.


================
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
----------------
ABataev wrote:
> `Type *`
Fixed


================
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();
----------------
ABataev wrote:
> Just `cast`
Fixed


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7140
+      CurrentType = AT->getElementType();
+    } else {
+      return false;
----------------
ABataev wrote:
> What about VectorType here if it is allowed in the previous function?
No need to do this: `insertvalue`'s indices can never point to vector elements, only to structure and array ones. Function `getAggregateSize()` doesn's use `InsertValueInst::indices()` and computes aggregate size by `getNumElements()` multiplication, whereas `getOperandIndex()` just returns `insertvalue`'s operand offset. The vector can be addressed by `insertvalue` only as final atomic element of structure. Function `findBuildAggregate_rec()` processes this element separately using `OperandIndex` as base offset.


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


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7163-7166
+    } else {
+      BuildVectorOpds[OperandIndex] = InsertedOperand;
+      InsertElts[OperandIndex] = LastInsertInst;
+    }
----------------
ABataev wrote:
> No need for `else` here
No, this `else` matters here: it embraces the block which is executed only if `InsertedOperand` is not `insert`. There is the second nested `if` inside the first one.


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


================
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(),
----------------
ABataev wrote:
> Just `llvm::erase_value(<container>, nullptr)`
Thanks, changed. Used `llvm::erase_if(<container>, <predicate>)`, didn't found `llvm::erase_value()`.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7225-7226
+    if (BuildVectorOpds.size() >= 2)
+      return true;
+  }
+
----------------
ABataev wrote:
> Tabs
Hmm, that's actually spaces, not tabs.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7326
   if (!findBuildAggregate(IEI, TTI, BuildVectorOpds, BuildVectorInsts) ||
-      BuildVectorOpds.size() < 2 ||
       (llvm::all_of(BuildVectorOpds,
----------------
ABataev wrote:
> Why you don't need this?
This is checked inside `findBuildAggregate()` itself now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83779