[PATCH] D96628: [VPlan] Add plain text (not DOT's digraph) dumps

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the latest updates!

I just have a few remaining comments. After those, I think this is good to go. If there are any concerns with the direction by others, it would be great to hear them soon.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7829
+    if (PrintVPlansInDotFormat)
+      O << *Plan;
+    else
----------------
I think it would make sense to flip things here, as in have the operator for `raw_ostream` use the non-dot style (because that's what most useful in debuggers too) and then have a `Plan::printWithDot()` or soemthing for the DOT logic.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:406
+  for (auto *Succ : getSuccessors())
+    O << Indent << " " << Succ->getName();
+  O << '\n';
----------------
I think it would be good to separate the block names by a comma or something like that. You could either use `join_items` or `ListSeparator` from `StringExtras.h`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1728
 
+  void print(raw_ostream &O) const;
+
----------------
nit: comment, something like `/// Print the plan to \p O.`. Some for other instances.


================
Comment at: llvm/test/Transforms/LoopVectorize/vplan-printing.ll:3
 
 ; RUN: opt -loop-vectorize -debug-only=loop-vectorize -force-vector-interleave=1 -force-vector-width=4 -prefer-inloop-reductions -disable-output %s 2>&1 | FileCheck %s
 
----------------
I think we should have at least a LIT test that also checks `-vplan-print-in-dot-format=true`. Perhaps this file would a good candidate to do so? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96628/new/

https://reviews.llvm.org/D96628