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

Vasileios Porpodas via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Sep 25 21:53:17 PDT 2018


vporpo added a comment.

Hi Florian,  yes I am happy with the changes, thanks!
Just please try to add an assertion that checks that the graph is a tree (see inline comment) to make sure that the graph is the one we expect.



================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:134
+
+      if (LoadsSeen > 0 && VPI->getOpcode() == Instruction::Store) {
+        LLVM_DEBUG(dbgs() << "VPSLP: Store between load\n");
----------------
fhahn wrote:
> 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?
I agree it looks better this way.


================
Comment at: lib/Transforms/Vectorize/VPlanSLP.cpp:345
+  auto I = BundleToCombined.find(to_vector<4>(Values));
+  if (I != BundleToCombined.end())
+    return I->second;
----------------
fhahn wrote:
> 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
Ah yes, you are checking for unique users, so it should work the way you are describing. If I understand correctly, the graph can still be considered a tree since the only case when two nodes are connected by more than one path is when the user has two identical operands.

If this is the case, I still think that there should be some kind of assertion here checking that the graph is a tree and not a DAG, because SLP on a DAG is slightly different and would need a few more components to work correctly. I think that the only case where (I != BundleToCombined.end()) is true is the one you are describing: when the user instructions have identical operands. So maybe add an assertion that checks that the single users of Values have identical operands and a comment like "For now buildGraph should form a tree, not a DAG.".



https://reviews.llvm.org/D49491





More information about the llvm-commits mailing list