[PATCH] D30731: [SLP] Visualize SLP trees with -view-slp-tree

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

Thank you, this will be incredibly helpful. I don't think it quite works, though - see inline.

(In any case, I can only review the SLP part, never implemented GraphTraits.)



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:517
+
+    /// The TreeEntry index containing the user of this entry.
+    int UserTreeIdx;
----------------
tl;dr: I think this is broken, but it's not something you can easily fix here.

Fair warning: I haven't had time to really think this through, so maybe there's something trivial I'm missing here. Having said that - I just realized yesterday the tree is a lie. It isn't actually a tree, it's a DAG - so "the user" is not well-defined (see line 1095 on the left, and the discussion about the trouble it causes here: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170306/435431.html )

I think the whole thing should be rewritten, to be explicit about the graph it contains. But right now, I believe the only correct way to look up which tree nodes feed a node T is the way vectorizeTree() does it - look at the (scalar) operands of T's scalars, and map them back to the nodes through ScalarToTreeEntry. I'm not sure trying to wrap GraphTraits around that is a good idea, though.

We can either:
1) Leave this as is, and be explicit about the fact that the rendered graph may be imprecise.
2) Try to walk the graph "properly".
3) Put this on hold until the underlying "graph" has a saner representation. I'm not sure when I'll have time to do it, though. If anybody else volunteers...

None of those sound appealing, TBH. Not sure what the least evil is.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2136
+  {
+    raw_string_ostream OS(Str);
+    OS << "SLP: Spill Cost = " << SpillCost << ".\n"
----------------
Any reason not to render this as part of the graph regardless of NDEBUG?


https://reviews.llvm.org/D30731