[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/VPlan.cpp:588
+  for (VPUser *User : users())
+    for (unsigned i = 0, e = User->getNumOperands(); i < e; i++)
+      if (User->getOperand(i) == this)
----------------
1. `i`->`I`, 'e'->`E` per coding standard.
2. Use `++I` instead of `i++` per coding standard


================
Comment at: lib/Transforms/Vectorize/VPlan.h:640
+
+  Instruction *getUnderlyingInstr() {
+    if (!getUnderlyingValue())
----------------
Make the function `const`


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1434
+
+  bool isCompletelySLP() { return CompletelySLP; }
+};
----------------
`const`


================
Comment at: lib/Transforms/Vectorize/VPlan.h:616-619
+  VPInstruction *clone() {
+    SmallVector<VPValue *, 2> Operands(operands());
+    return new VPInstruction(Opcode, Operands);
+  }
----------------
`VPInstruction *clone() const`


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1434
+public:
+  VPSlp(VPInterleavedAccessInfo &IAI) : BundleToCombined(), IAI(IAI) {}
+
----------------
No need to initialize `BundleToCombined` here, will be initalized by default


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:84
+
+bool VPSlp::areVectorizable(SmallVectorImpl<VPValue *> &Operands) {
+  // Currently we only support VPInstructions.
----------------
`const` member function


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:164-165
+
+SmallVector<SmallVector<VPValue *, 4>, 4>
+getOperands(SmallVectorImpl<VPValue *> &Values) {
+  SmallVector<SmallVector<VPValue *, 4>, 4> Result;
----------------
`static`


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:176
+    auto *VPI = cast<VPInstruction>(Values[0]);
+    for (unsigned i = 0, numOps = VPI->getNumOperands(); i < numOps; i++)
+      Result.push_back(getOperands(Values, i));
----------------
`i`->`I`, `numOps`->`NumOps`, `++I`


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:191
+      }))
+    return ~0u;
+  return Opcode;
----------------
Maybe it is better to use `llvm::Optional<unsigned>` instead of some magic numbers for non-matching case?


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:234
+
+std::pair<OpMode, VPValue *> getBest(OpMode Mode, VPValue *Last,
+                                     SmallVectorImpl<VPValue *> &Candidates,
----------------
`static`


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


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:287-292
+  for (auto I = Candidates.begin(), E = Candidates.end(); I != E; I++) {
+    if (*I == Best) {
+      Candidates.erase(I);
+      break;
+    }
+  }
----------------
`std::remove(Candidates.begin(), Candidates.end(), Best);`?


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:312
+
+  for (unsigned Lane = 1; Lane < MultiNodeOps[0].second.size(); Lane++) {
+    LLVM_DEBUG(dbgs() << "  Finding best value for lane " << Lane << "\n");
----------------
2. `Lane = 1, E = MultiNodeOps[0].second.size(); Lane < E;`
1. Use preincrement


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:324
+
+    for (unsigned Op = 0, E = MultiNodeOps.size(); Op < E; Op++) {
+      LLVM_DEBUG(dbgs() << "  Checking " << Op << "\n");
----------------
preincrement


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:330
+      VPValue *Last = FinalOrder[Op].second[Lane - 1];
+      auto Res = getBest(Mode[Op], Last, Candidates, IAI);
+      if (Res.second)
----------------
Please, use the actual type here


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:342
+
+static void dumpBundle(SmallVectorImpl<VPValue *> &Values) {
+  LLVM_DEBUG(dbgs() << " Ops: ");
----------------
`ArrayRef<VPValue *> Values`


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:398-400
+          if (CombinedOperands[i] == Ops.first) {
+            CombinedOperands[i] = NewOp;
+          }
----------------
No braces here


https://reviews.llvm.org/D49491