You should not have debugging information in code that is up for review. If this is debugging information that you plan to leave in for future purposes (which I doubt is the case here), you need to protect it so that it isn't active unless some option is set. For example, see LLVM_DEBUG code that lives in various opt routines.

LGTM. Thanks.

Sorry for the delay, missed this before going on vacation.

I'm wondering what would happen (after this change) if a function were to re-order the blocks (without any other change). EG, the first 2 blocks in the function are swapped in the function list of blocks with no other change. I suspect that this would then indicate that the change was the original block 1 was replaced with the new block 2 and the original block 2 was replaced with the new block 1 with the corresponding changes to the branches. I think that may be unavoidable since the comparisons are string based. Besides, it is better than crashing...

Since you have the before and after versions, I expect you could get -print-changed=diff and cdiff without too much difficulty by calling the diff routine that the opt print-changed options use, if you want to expand this functionality.

LGTM. Thanks.

In D128048#3607295, @MaskRay wrote:

You may add -ftime-trace=<file> instead of introducing a new spelling.

In D128048#3601588, @Whitney wrote:

In D128048#3601579, @jamieschmeiser wrote:

Can you please use git rebase -i to collapse all the changes into a single change? If this isn't done, it is difficult to know what is being reviewed as the changes only show the differences since your last revision, not all of the changes.

I can see all the changes, make sure your link is not suffix with "new", or check the history tab to see what is it comparing.

Can you please use git rebase -i to collapse all the changes into a single change? If this isn't done, it is difficult to know what is being reviewed as the changes only show the differences since your last revision, not all of the changes.

This is a good start. You should mention in the summary that when -c is not specified, the compiler is invoked with -o pointing at /tmp so the .json file will currently be placed there, which is confusing.

Respond to review comments: add cpp test.

Thanks!

Failing build: https://lab.llvm.org/buildbot/#/builders/42/builds/2800

It was suggested by @reames that -exec-on-ir-change=exe is a better name for the option. This sounds like a good suggestion to me so I have renamed it.

renamed option to -exec-on-ir-change

I see. Yes, that would be a nice feature.

Yes, it would be nice. BTW, the choice is not hard coded but is specified with DefaultDOTGraphTraits::renderNodesUsingHTML() and stored in the ctor since it is unlikely that an HTML and non-HTML mix would be desirable.

Yes, bugpoint will find a misbehaving class but that is just one example usage for this. test-changed is more general in that any script/exe can be called each time the IR changes. Consider the situation where a performance degradation is noticed so the code still compiles but is not as well optimized. bugpoint (as I understand from the documation) will not detect this because the IR still compiles and the exe still runs. Using test-changed and a script that calls llc followed by a timed execution, one could determine how much impact the various passes have on the performance of the code as it gets changed in the pipeline. This could point out the source(s) of speed-ups and slow-downs in the pipeline and could be used as an exploration tool.

Replace the shell script with just calling cat directly. cat is used in many lit tests and will cat the IR file and will give an error that no file exists with the name of the pass and this can be checked. @thakis, can you please verify that this does not break the windows build?

@aeubanks Thanks for reverting it. I'll take a look.

I've updated the patch with some changes to same parameter names (V -> Value, etc) and corresponding comments. The only substantive change in the update is that I changed DotCfgDiffNode::getEdge to DotCfgDiffNode::getEdgeColour and had it return a StringRef to the colour instead of the std::pair it previously did since the only use just extracted the colour from the pair (lines 1496 and 1769).

ping

The dot documentation I saw recommended this method of checking whether an output format was supported.

ping

Respond to review comments: remove templates in functions for temporary file creation and cleanup.

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

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

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

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?

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

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

missed @MaskRay 's comment, will add that before attempting a relanding

Failed the lit test.
https://lab.llvm.org/buildbot/#/builders/110/builds/6977
https://lab.llvm.org/buildbot/#/builders/36/builds/13137
https://lab.llvm.org/buildbot/#/builders/52/builds/11218

LGTM, thanks

As I recall, I added this in based on comments in the review and I am fine with removing it. However, the parameter to unWrapAndPrint was added to preserve existing function and if this is removed, then the parameter should also be removed as it will always be false. The parameter can be dropped from the call to print also, as it defaults to false.

Remove unnecessary include.

Added the usual filtering options (filter-passes and filter-print-func) with uninteresting passes being saved with a message that they were filtered out. This should speed up things significantly when they are used. If the user isn't aware of the name of the crashing pass, they can use -filter-passes=blah (where blah doesn't exist) and they will get a message saying that the pass was filtered out. They can discover the name from that message and then rerun it with -filter-passes.

I think I am getting confused with llvm-extract --func=<function> which will extract what is needed for a function from a module IR. However, one could likely use that to get an IR with the necessary addition declarations and then substitute in the IR for the function using this option. I wonder if it might be possible to use the code in llvm-extract to do that automatically? Something to think about...

Regarding always printing the module, I'm not sure that is desirable. The
problematic IR may be very large and the option respects -print-module-scope
so the module IR can be obtained. There are also tools for creating a fleshed-out IR
from the IR for a function.

As requested, I removed the mutex code and cleaned up a few things.

I agree that the mutex code is overkill, considering that this is intended for debugging. If things go off the rails because of the lacking mutex code, the consequences are not severe since it is expected to crash anyway (or else why would you use this?)
I will removed the mutex code and clean things up based on your comments unless someone raises an objection.

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.

fix Clang-tidy problems

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?

Update and change test pass to use assert(false)

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.

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

Add // NOLINTNEXTLINE comments

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

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

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

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

Trying again with a newly generated patch...

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.

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.