[PATCH] D49491: [RFC][VPlan, SLP] Add simple SLP analysis on top of VPlan.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:64
+
+VPInstruction *VPSlp::addCombined(SmallVector<VPValue *, 4> &Operands,
+                                  VPInstruction *New) {
----------------
ABataev wrote:
> fhahn wrote:
> > ABataev wrote:
> > > `ArrayRef<VPValue *> Operands`
> > Operands is used as key for the BundleToCombined map, which expects a SmallVector<VPValue *, 4> key. 
> llvm has special function `llvm::to_vector<4>()` that will allow to convert  from `ArrayRef` to `SmallVector`
Neat, thanks for pointing that out!


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:178-182
+  default: {
+    for (unsigned I = 0, NumOps = VPI->getNumOperands(); I < NumOps; ++I)
+      Result.push_back(getOperands(Values, I));
+    break;
+  }
----------------
ABataev wrote:
> Do we need braces here? 
Not anymore, thanks


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:302
+  for (auto &Operands : MultiNodeOps) {
+    FinalOrder.push_back({Operands.first, {Operands.second[0]}});
+    if (cast<VPInstruction>(Operands.second[0])->getOpcode() ==
----------------
ABataev wrote:
> ABataev wrote:
> > Is it possible to replace it with  `emplace_back(Operands.first, Operands.second[0]);`
> You can preallocate the memory for `Mode` and `FinalOrder` vectors, because you know the number of elements is `MultiNodeOps.size()`
I don't think so, it fails to deduce the correct type for the second argument (with and without braces)


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:334
+        // TODO: handle this case
+        FinalOrder[Op].second.push_back(markFailed());
+    }
----------------
ABataev wrote:
> use `emplace_back()`
Is there are benefit by using emplace_back here? We are not constructing any objects here I think


https://reviews.llvm.org/D49491