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.

[jamieschmeiser]

Summary

Add new options -print-changed=[dot-cfg | dot-cfg-quiet] which create
a website of DOT files showing colourized changes as the IR is changed
by passes in the new pass manager pipeline.

A new change reporter is introduced that creates a website of changes made
by passes in the opt pipeline that change the IR. The hidden option
-dot-cfg-dir=<dir> specifies a directory (defaulting to "./") into which the
website will be created.

A file passes.html is created that contains a list of all the passes that
act on the IR. Those that do not change the IR are listed as omitted
because of no change, ignored or filtered out (using -filter-print-func
and -filter-passes) or not listed in quiet mode. Those that
do change the IR are listed as a link to a DOT file which contains a
CFG depiction of the IR (ala -dot-cfg) except that the instructions,
basic blocks and links that are only in the IR before the pass (ie, removed)
and those that are only in the IR after the pass (ie, added) are shown in
red and green, respectively, while the aspects of the CFG that do not change
are shown in black. Additional hidden options
-dot-cfg-before-color=<dot named color>,
-dot-cfg-after-color=<dot named color> and
-dot-cfg-common-color=<dot named color> are defined that allow the
customization of the colors used in colorizing the CFG.
-change-printer-dot-path=<path to dot exe> is also added.

This builds upon the base classes introduced in
https://reviews.llvm.org/D86360 and expanded in
https://reviews.llvm.org/D87000 and https://reviews.llvm.org/D91890. The changes to the GraphWriter in
https://reviews.llvm.org/D86362 have been incorporated into this PR as they are
difficult to understand in isolation and that PR was cancelled.

This change printer was introduced during my tuturial at the October 2020
virtual developer's meeting.

[jamieschmeiser]

Don't apologize for "slow reviews," I appreciate your taking the time to review all of these changes. If there is anything I can do to make it easier, please let me know.

[aeubanks]

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.

llvm/include/llvm/Passes/StandardInstrumentations.h

274

BlockDataBase?

525

std::unique_ptr?

llvm/include/llvm/Support/DOTGraphTraits.h

71

is this param used anywhere?

llvm/lib/Passes/StandardInstrumentations.cpp

135

should probably be more specific, like dot-cfg-before-color

llvm/test/Other/ChangePrinters/print-changed-dot-cfg.ll

6 ↗

(On Diff #325763)

all these should probably use -dot-cfg-dir in a temp directory

7 ↗

(On Diff #325763)

are all the 2>&1 in here necessary? and > /dev/null

7 ↗

(On Diff #325763)

-disable-output

7 ↗

(On Diff #325763)

-S shouldn't be necessary

[aeubanks]

llvm/lib/Passes/StandardInstrumentations.cpp

37

looks like llvm doesn't really use <regex> a lot, and I vaguely remember hearing bad things about it.
Does llvm/include/llvm/Support/Regex.h work?

1442

we should really get this figured out

[jamieschmeiser]

I have addressed your comments from previous reviews. Regarding the statics to avoid a crash,
I figured out that the problem was a template instantiation that was missing a const, resulting
in a copy being made, which didn't copy everything. This has been fixed. I have also
simplified the code and templates.

[jamieschmeiser]

Trying newly generated patch as last one couldn't be applied. I also added code to call expand_tilde on the -dot-cfg-dir specified directory and then make it absolute.

[jamieschmeiser]

Trying again with a newly generated patch...

[jamieschmeiser]

Removed stale parent reviews because I think they were causing problems with applying the patch

[jamieschmeiser]

Fix lit test failure: make test unsupported if /usr/bin/dot is unavailable.

[jamieschmeiser]

Fix clang tidy problems and add option to specify dot executable.

[jamieschmeiser]

The clang-tidy complaints relate to names that are required by existing code.

[jamieschmeiser]

Add // NOLINTNEXTLINE comments

[jamieschmeiser]

make clang-tidy comments specific to readability-identifier-naming

[jamieschmeiser]

I do not know what is causing the debian build to fail but I do not believe the failure is caused by my changes. I used the instructions to replicate the failure and this resulted in the same error that llgo isn't a known project. Checking out the previous commit resulted in the same error so it is a pre-existing condition and not the result of my changes.

[aeubanks]

don't worry about the presubmit
I'll try to get around to reviewing this at some point (although it might take some time since this is a very large change)

[jamieschmeiser]

Thanks. Would it help if I split it up a bit? For example, there are a lot of changes relating to making data into templates which affects the existing change printers but doesn't change their functioning. These could be split out and handled first to simplify things. Would this help?

[aeubanks]

In D87202#3028295, @jamieschmeiser wrote:

Thanks. Would it help if I split it up a bit? For example, there are a lot of changes relating to making data into templates which affects the existing change printers but doesn't change their functioning. These could be split out and handled first to simplify things. Would this help?

Yes please

[jamieschmeiser]

Created new review https://reviews.llvm.org/D110737 with non-functional background changes to simplify this review. I will update this review to reflect any changes from that review when it lands.

[jamieschmeiser]

Update after landing of D110737 which had most of the changes to existing code.

[aeubanks]

this is impossible for me to review especially since I don't really know html super well
but lgtm (with nits) I guess, I tried patching this in and running it and it does work as expected

llvm/lib/Passes/StandardInstrumentations.cpp

1309

I've barely seen this anywhere in LLVM, I don't think it's necessary

llvm/test/Other/ChangePrinters/DotCfg/print-changed-dot-cfg.ll

134

I'd add -disable-verify to the opt invocations to avoid these, since it'll already implicitly be off in a release build

[jamieschmeiser]

Respond to review comments, remove nolint instructions and clean
up unit tests.

[tpopp]

Hi Jamie, this code has test failures: https://reviews.llvm.org/B131761

Additionally, you're not required to fix these build issues, but other issues might appear on some platforms as you can see here: https://buildkite.com/llvm-project/upstream-bazel-rbe/builds/11083#96df16f2-6dd8-4800-a2f1-25de809a99f1

[jamieschmeiser]

Hi @tpopp, can you please explain to me the nature of these failures? The code changed here is only active under options that are not being used in those tests so I expect that the errors are unrelated. I have not received any notifications from the build-bots of a problem so far. I am also unclear as to why there are build issues on those other platforms. Are you asking me to revert this?

[tpopp]

In D87202#3103520, @jamieschmeiser wrote:

Hi @tpopp, can you please explain to me the nature of these failures? The code changed here is only active under options that are not being used in those tests so I expect that the errors are unrelated. I have not received any notifications from the build-bots of a problem so far. I am also unclear as to why there are build issues on those other platforms. Are you asking me to revert this?

Please ignore the first link. I didn't look further at the first link, so they're probably existing failures.

The second test was for a different BUILD system, so normally other people are responsible for fixing them. In this case it showed code that doesn't work on all platforms, so I made a small modification to make this work. Hopefully it's okay: https://github.com/llvm/llvm-project/commit/88fc0ab45db98f097c5ef898068081eb097cceeb

If that change is okay, nothing more needs to be done.

[jamieschmeiser]

Hi @tpopp, yes, it looks fine. Thanks!

[hvdijk]

I am seeing:

********************
FAIL: LLVM :: Other/ChangePrinters/DotCfg/print-changed-dot-cfg.ll (6061 of 45994)
******************** TEST 'LLVM :: Other/ChangePrinters/DotCfg/print-changed-dot-cfg.ll' FAILED ********************
[...]
--
Exit Code: 1

Command Output (stderr):
--
Format: "pdf" not recognized. No formats found.
Perhaps "dot -c" needs to be run (with installer's privileges) to register the plugins?
Format: "pdf" not recognized. No formats found.
Perhaps "dot -c" needs to be run (with installer's privileges) to register the plugins?
Format: "pdf" not recognized. No formats found.
Perhaps "dot -c" needs to be run (with installer's privileges) to register the plugins?
ls: cannot access '/home/harald/llvm-project/build/test/Other/ChangePrinters/DotCfg/Output/print-changed-dot-cfg.ll.tmp/*.pdf': No such file or directory
Expected 4 lines, got 1.

--

http://graphviz.org/docs/outputs/pdf/ mentions that whether PDF output is supported by dot depends on how Graphviz was configured; we cannot assume that if dot is present, it will support PDF. Is there some way to ensure that in this case, the test is skipped?

[uabelho]

llvm/lib/Passes/StandardInstrumentations.cpp

1648

gcc warns about this function being unused:

[2174/4649] Building CXX object lib/Passes/CMakeFiles/LLVMPasses.dir/StandardInstrumentations.cpp.o
../lib/Passes/StandardInstrumentations.cpp:1649:13: warning: 'std::string {anonymous}::DotCfgDiff::attribute({anonymous}::IRChangeDiffType) const' defined but not used [-Wunused-function]
 1649 | std::string DotCfgDiff::attribute(IRChangeDiffType T) const {
      |             ^~~~~~~~~~

Will it be used or should it be removed (or should it be there even if it's unused)?

[jamieschmeiser]

Hi @hvdijk, a fix for the lit-test problem is up for review in https://reviews.llvm.org/D113187

[jamieschmeiser]

Hi @uabelho, the function is unnecessary and has been removed in https://reviews.llvm.org/D113188

[uabelho]

In D87202#3108860, @jamieschmeiser wrote:

Hi @uabelho, the function is unnecessary and has been removed in https://reviews.llvm.org/D113188

Cool, thanks!

[mehdi_amini]

llvm/lib/Passes/StandardInstrumentations.cpp

1267

This seems like this global is mutable and I see a bunch of TSAN failures, can you look into this?

[aeubanks]

llvm/lib/Passes/StandardInstrumentations.cpp

1267

Yeah this should probably be a member variable.

But where are you seeing tsan failures? This is used for debugging and the in-tree tests for this should only be single threaded.

[mehdi_amini]

llvm/lib/Passes/StandardInstrumentations.cpp

1267

WARNING: ThreadSanitizer: data race (pid=45032)
  Write of size 1 at 0x000005020700 by thread T2:
    #0 memcpy <null> (llvm-lto2+0x143fb6e)
    #1 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_assign(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) <null> (libstdc++.so.6+0x13c835)
    #2 runNewPMPasses /usr/local/google/home/aminim/projects/llvm-project/llvm/lib/LTO/LTOBackend.cpp:242:28 (llvm-lto2+0x268869a)
    #3 llvm::lto::opt(llvm::lto::Config const&, llvm::TargetMachine*, unsigned int, llvm::Module&, bool, llvm::ModuleSummaryIndex*, llvm::ModuleSummaryIndex const*, std::vector<unsigned char, std::allocator<unsigned char> > const&) /usr/local/google/home/aminim/projects/llvm-project/llvm/lib/LTO/LTOBackend.cpp:370:5 (llvm-lto2+0x268869a)

[jamieschmeiser]

llvm/lib/Passes/StandardInstrumentations.cpp

1267

This array is actually unnecessary and was only there to ease understanding of the code. I will remove it.

[jamieschmeiser]

llvm/lib/Passes/StandardInstrumentations.cpp

1267

@mehdi_amini Fix in https://reviews.llvm.org/D115175

[jrtc27]

This commit broke the non-HTML output, causing https://github.com/llvm/llvm-project/issues/52610

llvm/include/llvm/Support/GraphWriter.h

259

Surely you don't want this for HTML?

268–271

I believe this whole chunk of code is meant to be this

269

This makes no sense. It's not truncated if i != e?

271

The } was unconditional before, now it's conditional so you get mismatched }s and graphviz errors out unless you have more than 64 edge dest labels

[jamieschmeiser]

llvm/include/llvm/Support/GraphWriter.h

259

@jrtc27 This changes in this section are unnecessary because the code that uses the HTML rendering does not use edge destination labels. I will change this section back to what it was.