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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Some minor/style comments as I get up to speed on the code.



================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:588
+  for (VPUser *User : users()) {
+    for (unsigned i = 0; i < User->getNumOperands(); i++) {
+      if (User->getOperand(i) == this)
----------------
```
for (unsigned i = 0, e = User->getNumOperands(); i < e; i++) {
```


================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:592
+    }
+  }
+}
----------------
Remove unnecessary braces?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:644
+    assert(isa<Instruction>(getUnderlyingValue()));
+    return cast<Instruction>(getUnderlyingValue());
+  }
----------------
No need for assert - cast will catch it.

Could you use cast_or_null<Instruction>(getUnderlyingValue()) directly?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1425
+
+  SmallVector<MultiNodeOpTy, 4> reorderMultiNodeOps();
+
----------------
Please can you add description comments for all these member variables and public functions.


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:59
+
+unsigned getNumUniqueUsers(VPValue *V) {
+  SmallPtrSet<VPUser *, 4> Users(V->user_begin(), V->user_end());
----------------
static?


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:90
+      })) {
+    dbgs() << "SLP: not all operands are VPInstructions\n";
+    return false;
----------------
Should you use VPSLP for debug messages to avoid confusion with SLPVectorizer?


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:154
+  for (VPValue *V : Values) {
+    VPUser *U = cast<VPUser>(V);
+    Operands.push_back(U->getOperand(OperandIndex));
----------------
auto?


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:161
+static bool areCommutative(SmallVectorImpl<VPValue *> &Values) {
+  switch (cast<VPInstruction>(Values[0])->getOpcode()) {
+  case Instruction::Add:
----------------
Reuse Instruction::isCommutative(cast<VPInstruction>(Values[0])->getOpcode())?


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:182
+  case Instruction::Load:
+    assert(false);
+  case Instruction::Store:
----------------
llvm_unreachable?


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:189
+                  numOps = cast<VPInstruction>(Values[0])->getNumOperands();
+         i < numOps; i++)
+      Result.push_back(getOperands(Values, i));
----------------
This would be cleaner if you pulled out cast<VPInstruction>(Values[0])->getNumOperands()
```
unsigned NumOps = cast<VPInstruction>(Values[0])->getNumOperands();
for (unsigned i = 0; i < NumOps; ++i)
```


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:238
+
+  unsigned score = 0;
+  for (unsigned i = 0; i < cast<VPUser>(V1)->getNumOperands(); i++)
----------------
(style) Score


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:240
+  for (unsigned i = 0; i < cast<VPUser>(V1)->getNumOperands(); i++)
+    for (unsigned j = 0; j < cast<VPUser>(V2)->getNumOperands(); j++)
+      score += getLAScore(cast<VPUser>(V1)->getOperand(i),
----------------
Don't re-evaluate the getNumOperands() calls
```
for (unsigned i = 0, e1 = cast<VPUser>(V1)->getNumOperands(); j < e1; i++)
  for (unsigned j = 0, e2 = cast<VPUser>(V2)->getNumOperands(); j < e2; j++)
```


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:257
+    VPInstruction *LastI = cast<VPInstruction>(Last);
+    VPInstruction *CandidateI = cast<VPInstruction>(Candidate);
+    if (areConsecutiveOrMatch(LastI, CandidateI, IAI)) {
----------------
(style) auto for casts


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


https://reviews.llvm.org/D49491