Respond to review comments:
Establish an ordering based on the "after" ordering with the removed
sections interspersed. This ordering is applied to get the basic blocks
in the order they are in the function and also applied to the functions
to get them into the same order they are in the module.

Updated option control to be -print-changed=[diff | diff-quiet]

rather than a separate option.  This also introduces the
quiet mode.  Change/add tests accordingly.

A new quiet mode for change printers is being considered in https://reviews.llvm.org/D92589. I will likely have to change this patch based on the results of that, which would introduce a quiet mode into this change reporter also.

I would like to change the option handling for this based on the discussions in https://reviews.llvm.org/D92589. In particular, I was thinking that this option could be controlled using -print-changed=diff and -print-changed=diff-quiet rather than using a separate option. WDYT? I will wait until that patch is settled before making further changes to this one.

I have changed the option handling as discussed and used the optional
string on the option as you suggested. I have also changed this from
being a separate change printer to being an option on the base class.
This way, they other proposed change printers can also benefit by getting
the verbose/quiet modes using the same code.

I was thinking about a slightly different approach for these options. There are several different options (-print-changed, print-changed-diff, -dot-cfg-changed ) with different variations. As you say, it doesn't make sense to use more than one at a time so I was thinking that perhaps there could be -print-changed, and -print-changed-options=<options>. Without -print-changed-options, you would get this one (ie, before and after without banners for no changed, ignored, etc). The options would be a string of characters v being verbose (all banners), b is -print-before-changed, p is patch format (-print-changed-diff which could also have a verbose mode as currently up for review and default to skipping other banners). -dot-cfg-changed (also with/without verbose mode) would be d=string, b=string, a=string, c=string with strings going to the end of input or comma where d, b, a and c being the directory, before colour, after colour and common colour. The option letters can change, the important thing is the idea of the first option saying you want some form of change reporter (defaulting to this nonverbose print-changed version) and a modifier option that picks and controls the various types of reporter. WDYT?

@yrouban, @aeubanks indicated that he is satisfied but would like you to sign off on the signal handler.

I have removed the templates and the compare hierarchy
because they are not necessary for this change reporter.
They will have to be re-introduced for the final reporter as
it builds on this one but their removal simplifies this review.

LGTM

I previously saw unrelated changes showing up in the differences here but this no longer seems to be the case so that comment can be ignored.

Remove changes pertaining to legacy pass manager.

Respond to review comments: change option name to print-changed-diff,
change function name and change code to use ExecuteAndWait.

I see you have made the requested changes (nit: clang-tidy complained about the capitalization of the function) but why are there so many other, unrelated changes shown? Is there a problem with the patch?

I agree that having the callbacks ask for the names is an improvement as it is cleaner and allows other callbacks to use this feature if desired. Generally, things look good. I have a couple of minor concerns mentioned in the code but I think it would be acceptable as is if you don't agree with what I've mentioned.

The template base classes are also used for -dot-cfg-changes, which shows the diff by making a website and showing the changes in dot-cfg form with the added code shown in green and the deleted code shown in red. The template parameters are the data structures needed for that display, while they are just empty structs in this change reporter. https://reviews.llvm.org/D87202 has the changes for that change reporter but the changes are out-of-date as they build on the original patch for this change reporter (which has been split up), but the data structures used by the templates for that change reporter can be seen in that patch.

Changes based on clang-tidy/format

Rebasing code on latest master before delivering

@nikic I have moved the test for profile data before the calculation of the alias sets.

@nikic, saw your second comment after I added mine. I will revert it and take a look.

@nikic, this is strange because it is off by default. I've already reverted the revert, so I will check the time differences between the relanding and if it shows a regression again, I will pull it.

Looks like a flaky testcase, it passed on next run with my changes in on one build and the other looks like a network issue.

The Buildbot has detected a failed build on builder clang-cmake-armv7-quick while building llvm.
Full details are available at:

https://urldefense.proofpoint.com/v2/url?u=http-3A__lab.llvm.org-3A8011_-23builders_107_builds_1659&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=Nr2SnYQn80wzIpml4veK2C-U4ilVZzi0kWYCc2WSLoA&m=v5KeUvUQ3NmVKfDF_PHQ197pdpgdKEZ2Cm6JUebPiEs&s=QQkrhJP0JJLKRQfOOzGYX0pkct6JJhzL0CHRaZMbkEE&e=

Buildbot URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__lab.llvm.org-3A8011_&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=Nr2SnYQn80wzIpml4veK2C-U4ilVZzi0kWYCc2WSLoA&m=v5KeUvUQ3NmVKfDF_PHQ197pdpgdKEZ2Cm6JUebPiEs&s=_gM68RF9ovfGi9Cd5Z7GlMzchqp751e2xZaWYOIO8Ts&e=
Worker for this Build: linaro-armv7-quick
Build Reason: <unknown>
Blamelist: Jamie Schmeiser <schmeise@ca.ibm.com>
BUILD FAILED: failed 49489 expected passes 19381 unsupported tests 75 expected failures 1 unexpected failures Unexpected test result output 14 (failure)
Sincerely,
LLVM Buildbot

test failures when pushing

Fix build problems revealed when delivering changes.

No changes, just rebasing to new master to get retesting to ensure clean test run

Respond to review comments: move function out of line, remove inline keyword,
remove unneeded qualifiers, simplify comparison.

Respond to review comments: replace header include.

Respond to review comments: properly indicate preserved analyses,
which also means the changes are not needed for the pipeline tests.

Respond to review comments: use llvm::function_ref, add unit test.
Fix option name and MemoryUse search string.
Regarding "-> void", I prefer to have the return type specified.

As requested, I have broken out the refactoring aspects of the original PR.
I have placed them in this PR and will make a subsequent PR with the new
functionality of print-changes once these refactoring changes have
stabilized/landed to avoid churn in a chain of PRs. There are no intended
changes to functionality in this PR.

Respond to review comments by adding options controlling whether
memoryssa is used for loopsink, defaulting to false. Expanded the
tests to include these options.

Respond to review comments. Enable Memory SSA in legacy Loop Sink pass
under EnableMSSALoopDependency option control. Update tests accordingly.

As requested, I have split the refactoring of the SinkAndHoistLICMFlags
into a separate PR and made this PR a child of that.

Moved the tests into a separate directory and added a lit.local.cfg that tests
whether the needed diff support is available. The tests are flagged as
unsupported when the needed support is unavailable. Also, the tests
were expanded to check more of the results.

@aeubanks, it appears that you are correct that the previous error was not caused by my changes as I cannot reproduce it locally. I have changed the option back to -print-before-changed.

Note that @sidbav made contributions to these changes.

The changes are specific to -print-before and -print-after (which is the intended target and this work originated before -print-changed) but could the change be made universal? I would rather see the existing StringRef passed to all the callbacks changed rather than adding the new StringRef to specific callbacks. This way all passes benefit and are consistent. In particular, I would like to see this change applied to -printChanged; this came up in the reviews for that PR. If you would rather not make it universal, then at least change all of the callbacks to also receive the new StringRef.

@aeubanks, I agree but when I first put up this PR, there was a lit test failure and looking at the run command for the test, it had "/fuchsia.cpp -### -no-canonical-prefixes" in it, so I think the problem is that the existing -print-before is a prefix of -print-before-changed. How about -also-print-before-changed?

@aeubanks, just want to point out that I have changed the option name to avoid the problem of -print-before and -print-before-changed having a common prefix. The option name is now -also-print-before. Please verify that this is acceptable and then I will deliver this.

Changed name of option to -also-print-before to avoid common option prefix problem.

ping

ping

Made changes requested by @aeubanks, who is satisfied but would like @yrouban to check the signal handler.

The changes in this PR have been incorporated into https://reviews.llvm.org/D87202 as they are easier to understand in the context of those changes.

Update the code based on changes in the earlier PRs in the
chain. Also, 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 will be cancelled.

I will be updating this to reflect changes to PRs earlier in the chain.

Removed unnecessary template parameter and improved testing.

Improved comments, changed order of classes in header file,
changed error handling to produce error messages rather
than asserting when performing the linux diff operation.

Update PR based on print-changed which has now landed.
Do not move the code into a different file as I previously did
as it made it difficult to see the changes.

@ychen, I changed the tests so that the banners check the names of the passes, based on other tests I found that also rely on pass ids. The other tests use regex to handle template arguments and specify other pass ids directly, so that is what I changed the tests to do.
@MaskRay I believe your concerns have been addressed; if not, please let me know.

Put text in tests that indicates the name of the pass in the banner,
using regex for template arguments.

Note that the clang-tidy comment about the non-existent header is because this PR builds upon other PRs that have not been incorporated yet, one of which introduces the header file.

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.