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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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");
----------------
vporpo wrote:
> 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().
I've added a simple implementation for mayWriteToMemory to VPInstruction. We could just use mayWriteToMemory of the underlying instruction, but I think this would unnecessarily increase coupling of VPInstruction and IR instructions. What do you think?


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:140
+  }
+  return true;
+}
----------------
vporpo wrote:
> 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.
> 
I've added a check for (i) and I'll add a check for (ii) tomorrow. I'll think a bit more what the best way to do it would be.

I've added a unit test for (i), and will add IR tests once we VPlan SLP is hooked up to the vplan native code path.


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:345
+  auto I = BundleToCombined.find(to_vector<4>(Values));
+  if (I != BundleToCombined.end())
+    return I->second;
----------------
vporpo wrote:
> 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.
Currently we check for unique users. For example, in the testSlpReuse_1 we have 2 add instruction which add the same value. In that case we should re-use the already created combined instruction I think


https://reviews.llvm.org/D49491