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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:64
+
+VPInstruction *VPSlp::addCombined(SmallVector<VPValue *, 4> &Operands,
+                                  VPInstruction *New) {
----------------
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`


================
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;
+  }
----------------
Do we need braces here? 


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:194
+      }))
+    return {};
+  return {Opcode};
----------------
Use `llvm::None` instead


================
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() ==
----------------
Is it possible to replace it with  `emplace_back(Operands.first, Operands.second[0]);`


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:302-308
+    FinalOrder.push_back({Operands.first, {Operands.second[0]}});
+    if (cast<VPInstruction>(Operands.second[0])->getOpcode() ==
+        Instruction::Load)
+      Mode.push_back(OpMode::Load);
+    else
+      Mode.push_back(OpMode::Opcode);
+  }
----------------
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()`


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:318
+                 << " ");
+      Candidates.push_back(Ops.second[Lane]);
+    }
----------------
Also can preallocate the memory for the vector


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:334
+        // TODO: handle this case
+        FinalOrder[Op].second.push_back(markFailed());
+    }
----------------
use `emplace_back()`


https://reviews.llvm.org/D49491