[PATCH] D80711: Utility to dump .dot representation of SelectionDAG without firing viewer

[Madhur Amilkanthwar via Phabricator via llvm-commits]

madhur13490 marked 5 inline comments as done.
madhur13490 added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAG.h:442
+  /// Error will be emitted if the path is insane.
+  void dumpDotGraph(const std::string &FileName, const Twine &Title);
+
----------------
arsenm wrote:
> I would expect the filename to be a StringRef (I guess debuggers struggle more with this, and the others are using std::string)
In fact, I am preferring Twine here because I may want to concat input function name, code function name and some state. e.g filename = MachineFunction.getName() + "LowerCall"+"start" to indicate DAG for machine function in LowerCall at the start!


================
Comment at: llvm/include/llvm/Support/GraphWriter.h:344-345
       return "";
+    } else {
+      errs() << "writing to the newly created file " << Filename << "\n";
     }
----------------
arsenm wrote:
> The existing done I think was sufficient
No, just "done" doesn't indicate who's and what's done? There has to be some context to it and it is present with view* methods. So, I'd prefer emitting more verbose text.


================
Comment at: llvm/include/llvm/Support/GraphWriter.h:366-369
+  std::string Filename = llvm::WriteGraph(G, Name, ShortNames, Title, FileName);
+
+  if (Filename.empty())
+    return;
----------------
arsenm wrote:
> Don't see the point of getting the return string and check if it's empty?
Yeah. Removed.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGPrinter.cpp:171
+// This code is only for debugging!
+#ifndef NDEBUG
+  DumpDotGraph(this, FileName, Title);
----------------
arsenm wrote:
> Should this fully remove the code depending on LLVM_ENABLE_DUMP? Also the declaration should get LLVM_DUMP_METHOD?
Sure, the method will be more accessible in that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80711