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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 5 inline comments as done.
fhahn added a comment.

Address review comments, thanks!



================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:262
+    unsigned BestScore = 0;
+    for (unsigned Depth = 1; Depth < 5; Depth++) {
+      unsigned PrevScore = ~0u;
----------------
ABataev wrote:
> Why `5`? Use enum or const instead of magic number.
Replaced with a variable


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:64
+
+VPInstruction *VPSlp::addCombined(SmallVector<VPValue *, 4> &Operands,
+                                  VPInstruction *New) {
----------------
ABataev wrote:
> `ArrayRef<VPValue *> Operands`
Operands is used as key for the BundleToCombined map, which expects a SmallVector<VPValue *, 4> key. 


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:235
+static std::pair<OpMode, VPValue *>
+getBest(OpMode Mode, VPValue *Last, SmallVectorImpl<VPValue *> &Candidates,
+        VPInterleavedAccessInfo &IAI) {
----------------
ABataev wrote:
> `ArrayRef<VPValue *> Candidates`
We remove elements from Candidates which is not possible with ArrayRef I think


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:348
+
+VPInstruction *VPSlp::buildGraph(SmallVector<VPValue *, 4> &Values) {
+  assert(!Values.empty() && "Need some operands!");
----------------
ABataev wrote:
> `ArrayRef<VPValue *> Values`
Values is used as key for the BundleToCombined map, which expects a SmallVector<VPValue *, 4> key. 


https://reviews.llvm.org/D49491