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

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:134
+
+      if (LoadsSeen > 0 && VPI->getOpcode() == Instruction::Store) {
+        LLVM_DEBUG(dbgs() << "VPSLP: Store between load\n");
----------------
Could we have other non-Store VPInstructions that could touch memory (e.g., calls) ?
Maybe it is safer to add a check whether the underlying IR instruction mayWriteToMemory().


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:140
+  }
+  return true;
+}
----------------
I think we should also check that the operands are (i) all in the same BB, and (ii) in the same BB as the seed instructions (at least for now). A lit test for this would also be nice.



================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:345
+  auto I = BundleToCombined.find(to_vector<4>(Values));
+  if (I != BundleToCombined.end())
+    return I->second;
----------------
If I am not mistaken, since we are only allowing single user nodes in the graph, the graph is actually a tree. So this should not happen. We should have an assertion here to check this.


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:350
+  LLVM_DEBUG(dbgs() << "buildGraph");
+  dumpBundle(Values);
+  if (!areVectorizable(Values))
----------------
Use LLVM_DEBUG for dumpBundle()


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:363
+      LLVM_DEBUG(dbgs() << "  Visiting Commutative");
+      dumpBundle(Operands);
+      auto OperandsOpcode = getOpcode(Operands);
----------------
Use LLVM_DEBUG for dumpBundle()


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:405
+    if (ValuesOpcode == Instruction::Store)
+      CombinedOperands.push_back(cast<VPInstruction>(Values[0])->getOperand(1));
+  }
----------------
Hmm why no buildGraph() recursion here ?


https://reviews.llvm.org/D49491