[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
llvm-commits at lists.llvm.org
Mon Mar 1 10:17:37 PST 2021
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
More information about the llvm-commits
mailing list