Respond to review comments: use regex, format HTML output to make it easier
to read, other changes.

Fair enough, I'll come up with a regex that works for all three. I suspect that this will need to change in response to https://reviews.llvm.org/D87216

Update this PR to match changes made to https://reviews.llvm.org/D86360,
most notably replace the lambdas with virtual functions.
Adapt to other changes based on reviews of -print-changed.

@MaskRay, https://reviews.llvm.org/D87000 moves a bunch of the code into a separate file, rather than deleting it. -print-changed, by itself, doesn't add enough to justify it's own file but once you add in -print-changes and -dot-cfg-changes, it grows to a significant amount of code, so I moved it to a separate file. I had already marked the other PRs as children in a chain, which can be seen in the stack tab of the revision contents. Is there another way that I should indicate the relationship between the PRs? Thanks for your comments about these 3 PRs. I have found these ways of looking at the changes very useful, particularly -dot-cfg-changes. (Shameless promotion alert:) I will be presenting these three ways of looking at the IR in my tutorial at the upcoming virtual developers meeting, including a demo of -dot-cfg-changes.

Respond to review comments: change comments, few code revisions, notably with
std::string. Tighten up test cases with more -NEXT checks of the IR.

@ychen, Why are the regexs not sufficient for testing? I would rather not "check the common sub-string of GCC's and MSVC's output" because it has already been sufficiently demonstrated that this is fragile in the test. Referring to the code creating one of the banners:
SmallString<20> Banner = formatv("* IR Dump After {0} *", PassID);
We see that the test checks everything except the PassID (the name is added after) and the rest of the banner is distinct in all cases. I have changed the regexs to {{.+}} which will ensure that there is actually something there but checking anything else will make the test susceptible to failing based on the compiler. The important part of the test is captured in that the banner shows that the dump is either filtered, ignored, output, etc and that the portion of IR is correct. This is the intent of the tests, rather than testing how different compilers construct a PassID.
@MaskRay From earlier comments:
"The reason for the round-trip structure is that there are more instantiations of these templates that report changes in different forms still to come. I will be subsequently posting reviews for at least 2 different change reporters based on these base classes. By defining these templates in this fashion, the functionality of comparing different IR representations and determining when changes have occurred and should be filtered, reported or ignored is all in common in the base classes. The derived classes will be called when interesting events happen and only need to handle presenting the events without having to determine when the events occur."
There are 2 other derived change reporters currently up for review based on these base classes, although the PRs need updating based on changes made in this PR. This makes at least 3 derived classes. See https://reviews.llvm.org/D87000 and https://reviews.llvm.org/D87202

Respond to review comments: replace unneeded include with standard include, move code in file.
React to review comments on other PRs: change test prefixes to something more useful.
React to test failures with other PRs: change tests to have regex rather than having PassID in test
Add another test to test interaction with -print-module-scope

Respond to build/test problems: Remove extra ;, remove call to base dtor,
change tests to have regex for PassID and ModuleID

It appears that the failing tests are due to differences in the PassID that is passed in the PassInstrumentationCallbacks, which is presumably build compiler/platform dependent. I know that the name sent is language dependent because it is the mangled name for C++ but all of these tests are based on input that was originally C so that should be fine. I will alter the tests to have a regex accepting anything for the PassID and just verify the constructed part of the banner and the name, which should make these tests robust and still test the functionality.

@hliao Thanks!

@mkitzan, Thanks for fixing the build by removing the extra semicolon. Also, removing the call to the base dtor is fine.
commit 6859d95ea2d0f3fe0de2923a3f642170e66a1a14
Author: Michael Liao <michael.hliao@gmail.com>
Date: Wed Sep 16 14:43:08 2020 -0400

[NFC] Respond to reviewers comments: Update comments.

Respond to review comments: change lambdas to virtual function calls and
place code directly in functions.

Respond to review comments: Set ShouldPreserveUseListOrder to true
when generating the output for a module.

Change name to dot-cfg-changes as it is more similar to print-changes than to print-changed

Change option name to dot-cfg-changed to align with existing dot-cfg pass.

Changed code to only open output stream when option is specified.

@madhur13490 I have added a PR https://reviews.llvm.org/D87202 that the contains the code that uses the changes in this PR (and marked this as a parent of it).

Move enum out of template, make space between end template delimiters
optional in new test.

This is an update based on changes that have been made to the code in the review
for https://reviews.llvm.org/D86360. I have moved much of the code and templates into
a new header and body files as it is becoming more complex. I have also changed
some of the class names from being "change printer" based to "change reporter"
based because it doesn't imply how the changes are shown (ie, the changes do not
necessarily need to be printed).

Respond to review comments: Change global vector to pointer to DenseSet,
change objects from unnamed namespace to private statics.

Respond to review comments: Change template parameter name, use
llvm::isSpecialPass, make space between template end delimiters optional
and use more meaningful check prefixes in tests.

Merge of last 2 changesets for committing.

Thanks for doing the reviews.

Respond to review comments: fix comment and remove unnecessary test
from isInterestingFunction. Also, mark some functions inline.

Rebase to pick up review changes for https://reviews.llvm.org/D86360 and
alter this code to match those changes (reordered parameters, added const
to parameters)

[NFC] Respond to review comments: change parameter order, change variable
names, add const to parameters

Note that this code makes use of the linux diff function to determine what has changed in the IR. This will likely require altering for other platforms, which is a future enhancement. The testing may require attention before this PR can land and any suggestions on how to handle this would be appreciated.

I think you have to be logged in to the llvm conference website to follow the link. The abstract is available on the conference website.

Respond to review comments: remove dummy passes created in last
changeset and change tests to use no-op-function and instsimplify.

Respond to review comments: use mutex to protect building of vector of crash printers.
Create a global vector of crash printers that the signal handler uses to report the IRs in the event of a crash.
Adding a crash printer and removing it from the vector is protected using mutex locking.

Create 2 dummy passes, one that changes the IR by adding a dummy attribute
to a function, the other which does not change the IR. Neither are
added to any pipeline. Change the -print-changed unit test to use these
2 passes to make the unit test robust.

Address review comments.

Ping

For a test, I could create a pass that calls __builtin_trap that is only added to the opt pipeline by a really hidden option. The danger is that if someone uses that option, the compiler would crash, which seems dangerous just for testing. Is that desirable?

[NFC] Reorder includes and change case of variable as suggested by
clang-tidy/clang-format.

--print-before-all will print the IR before every pass which can result in the IR being printed literally thousands of times while this option prints it only once. The IR is saved in a string which is cleared each time and re-used so the memory overhead is not great; also this overhead is only hit when the option is requested. It does slow the compilation when used but not as much as --print-before-all and the user does not have as difficult a task in extracting the IR from the output. If the option is false (the default), the overhead is only the checking of the option. Basically, --print-before-all is cumbersome for this purpose while this option is convenient. Also, one could reasonably request a user to retrieve the IR (if they are willing) with this option when reporting a crash while requesting a user to use --print-before-all would not be reasonable.

This option will be introduced (along with others) in my tutorial at the LLVM developer's conference in October.

See comments regarding the unit tests

Thanks for the review comments. I have tried to address all of your concerns above and in the last set of changes posted.

Make IRChangePrinter a member of StandardInstrumentations to control
object lifespan.

I noticed a problem in the code and posted the fix separately as I did not want to confuse it with the changes based on the review comments.

Always stack an entry on the stack before each pass in change reporters.

Change function names based on suggestions from clang-tidy

Incorporate formatting changes suggested by clang-format/clang-tidy.