[PATCH] D87202: Add new choices dot-cfg and dot-cfg-quiet to print-changed which creates a website of DOT files showing colourized changes as the IR is changed by passes in the new pass manager pipeline.

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

In the description I think you meant `dot-cfg-dir` instead of `dot-cfg-changes`.

Again, lots of unnecessary `llvm::`.

I don't think I can really review the html generation too much, so I'll assume it's good unless somebody else wants to review it.



================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:262
 // before and after the pass to determine if it was changed by a pass.
-class ChangedBlockData {
+template <typename BlockDataT> class BlockDataTemplate {
 public:
----------------
`BlockDataBase`?


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:551
+  unsigned N = 0;
+  raw_fd_ostream *HTML = nullptr;
+};
----------------
`std::unique_ptr`?


================
Comment at: llvm/include/llvm/Support/DOTGraphTraits.h:71
+  // and the edge source labels.
+  static bool renderNodeUsingHTML(const void *) { return false; }
+
----------------
is this param used anywhere?


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:134
+// https://graphviz.org/pdf/dotguide.pdf
+cl::opt<std::string> BeforeColour("before-color",
+                                  cl::desc("Color for before elements."),
----------------
should probably be more specific, like `dot-cfg-before-color`


================
Comment at: llvm/test/Other/ChangePrinters/print-changed-dot-cfg.ll:6
+; Simple functionality check.
+; RUN: rm -f diff_*.pdf
+; RUN: opt -S -print-changed=dot-cfg -passes=instsimplify 2>&1 -o /dev/null < %s 2>&1 > /dev/null
----------------
all these should probably use `-dot-cfg-dir` in a temp directory


================
Comment at: llvm/test/Other/ChangePrinters/print-changed-dot-cfg.ll:7
+; RUN: rm -f diff_*.pdf
+; RUN: opt -S -print-changed=dot-cfg -passes=instsimplify 2>&1 -o /dev/null < %s 2>&1 > /dev/null
+; RUN: ls *.pdf | count 4
----------------
are all the `2>&1` in here necessary? and `> /dev/null`


================
Comment at: llvm/test/Other/ChangePrinters/print-changed-dot-cfg.ll:7
+; RUN: rm -f diff_*.pdf
+; RUN: opt -S -print-changed=dot-cfg -passes=instsimplify 2>&1 -o /dev/null < %s 2>&1 > /dev/null
+; RUN: ls *.pdf | count 4
----------------
aeubanks wrote:
> are all the `2>&1` in here necessary? and `> /dev/null`
`-disable-output`


================
Comment at: llvm/test/Other/ChangePrinters/print-changed-dot-cfg.ll:7
+; RUN: rm -f diff_*.pdf
+; RUN: opt -S -print-changed=dot-cfg -passes=instsimplify 2>&1 -o /dev/null < %s 2>&1 > /dev/null
+; RUN: ls *.pdf | count 4
----------------
aeubanks wrote:
> aeubanks wrote:
> > are all the `2>&1` in here necessary? and `> /dev/null`
> `-disable-output`
`-S` shouldn't be necessary


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

https://reviews.llvm.org/D87202