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

[Andrei Elovikov via Phabricator via llvm-commits]

a.elovikov added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:53
+cl::opt<bool>
+    PrintExecutedVPlan("print-executed-vplan", cl::init(false), cl::Hidden,
+                       cl::desc("Dump the VPlan that is being executed to "
----------------
fhahn wrote:
> I think having an option to just print the plans make sense, but as a first step, should we start with an option to just toggle between dot printing and regular printing for when using `-debug`?
I preferred dedicated option for several reasons:

1) Toggling is harder to implement, especially when caring about consistent interfaces. For example, I think that `VPBasicBlock`|`VPValue`|`VPDef`::`print` should be dedicated for text prints only (and `VPBasicBlock` can't even be printed as a digraph before this patch), so the only logical place for triggering would be `operator<<(ostream&)`. On the other hand, there is no guarantees that none of the LLVM_DEBUG prints call the print method directly (and not via stream operator).

2) I personally don't like using -debug/-debug-only for LIT testing purposes. Having a more limited/specific output is, IMO, preferable. I'm hoping that option(s) to print VPlan at a given place in the pipeline would be convenient tools for writing LIT tests. As such, I think having this option from the very first plain dump patch makes sense.

If going through the toggling approach, do you expect existing LIT tests (non-unittest) to continue using digraph output or be switched to plain dumps in the first patch? If the former, what would be your suggestion to test the plain dumps?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:866
+  // Use no indentation as we need to wrap the lines into quotes ourselves.
+  BasicBlock->print(SS, "", SlotTracker);
+  SmallVector<StringRef, 0> Lines;
----------------
fhahn wrote:
> Could you add a comment explaining that we first print the block and then split up/reconstruct the output with the .dot syntax>
Sure, will update in the next patch set.


================
Comment at: llvm/test/Transforms/LoopVectorize/icmp-uniforms.ll:40
+; CHECK-NEXT: loop:
+; CHECK-NEXT:   WIDEN-INDUCTION %iv = phi %bc.resume.val, %iv.next
+; CHECK-NEXT:   WIDEN ir<%cond0> = icmp ir<%iv>, ir<13>
----------------
fhahn wrote:
> Did the value here change because the plan gets printed later?
I didn't study in details, but I'd expect it to be so. Is that something expected for you, or do you want me to study that change in details (my knowledge of the VPlan pipeline is still limited)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96628