ping

@rsmith I separated the C and C++ messages in response to your comments and changed the C++ message to state that "performing pointer subtraction with a null pointer may have undefined behavior" to address your concerns. Are you satisfied? @thakis, the warning no longer fires in the MS headers according to @hans and there is separate option control over this warning. Is this sufficient? @efriedma, thank you for reviewing/approving the original but these changes were sufficiently different that I thought a new review would be beneficial. Are you still satisfied with the resulting changes?

ping

The reason I worded it with 'may' is because, in C++, nullptr - nullptr is defined. If the code is "nullptr - p" or "p - nullptr", it is only undefined behaviour when p is not nullptr, hence the 'may' part of the warning because this is not known at compile time. The warning is still useful as it is suspect code but one cannot state that it is undefined behaviour because it is valid if it is null at runtime.

ping

@thakis, can you please verify that the changes no longer affect the MS headers? Thanks.

Fix formatting.

Remove unnecessary tests.

Expand testing.

Significant changes made since previously accepted.

As requested, I have added a new warning option -Wnull-pointer-subtraction (and added it to -Wextra) that does not trigger on system headers. In addition, I changed the message used such that it states that it is undefined behaviour for C but may be undefined behaviour in C++ to address the concerns that performing the subtraction with a pointer that dynamically becomes null is not undefined behaviour. Expanded the testing to also test that the warning does not fire on system headers. Also, updated the release notes to indicate the new option.

Re-opening because it was reverted.

If this is a problem for you, please revert it and I will take a look when I return. Thanks.

@thakis, I have some questions about your comments.
Are you sure this is coming from a system header? The path that you gave has third_party as a directory in the path. If the warning were being triggered by code in a system header, I would have expected it to fire on something in the windows build bots but they appear to be clean.
I don't know if it is possible to suppress a warning based on whether the source is in a system header, or from a macro expansion that is defined in a system header; are you aware about whether or not this is possible? If it is, I suspect it would already be in force as a general setting, again making me question whether this is a system header...and if it isn't a system header, that wouldn't help you in any case.
If I understand your second point correctly, you have a system that previously compiled cleanly with warnings being treated as errors and you are concerned that if you use the existing options to suppress this particular warning, you are concerned that something could creep in. Therefore you are proposing a different warning group that would be a subgroup of the existing one so that if you set it, you would set both but you would still be able to set the specific one. I don't know if this is possible or not. Either way, I suspect that it would require using a different warning for the subtraction case, which would also require significantly changing these changes. Am I understanding correctly? If so, this implies that you have access to a workaround for your problem, although it may not be the best solution.
I do not have access to a windows setup to test any of these proposed changes; in particular, given that I suspect that the affected files are specific to some third party vendor from which you have purchased code, I do not have means of investigating the actual problems/solutions. If it would be helpful, I would be happy to review any changes that you might like to make to remedy the situation.
I will be on vacation for the next few days so please excuse my delayed responses.

ping

Respond to review comments: add C++ test.

ping

Limit tests to platform that supports altivec.

ping

LGTM

Thanks for doing this. There are a couple of comments/asserts that were missed...The only real thing is that the functionality of unwrapping Any is repeated many times... perhaps a function taking lambdas for what to do with each type of IR would be useful, probably in the header with Any? WDYT?

LGTM

Do you think that the IR being deleted should be reported rather than silently disappearing from the trace? Perhaps with

Out << "*** IR Deleted" << Name << " ***\n"; 
```before the return?

Fix indenting.

Respond to review comments: Do not issue warning for nullptr - nullptr in C++.

They already are removed; see the clean up section of code at the end of the function.

Respond to review comments: add requested test.

I suspect it may have something to do with name collisions in the calls to create a unique file name. It says that it loops a set number of times trying permutations and I think it just runs out of permutations, partly because the name doesn't have special characters allowing extra permutations. The uses of the file cannot collide (unless the pass manager becomes multi-threaded) so static solves the problem.

Reformat to satisfy clang-format

Added more reviewers.

The build problem is that __builtin_trap is not recognized in PassBuilder.cpp.

The Buildbot has detected a failed build on builder mlir-windows while building llvm.

Rebased and updated call to registerBeforeNonSkippedPassCallback. Also removed extra blank line at end of test.

I used formatting similar to the existing code, which is not what clang-format is expecting.

Yes, it is ready to land. There was a fair bit of discussion during the review so I left it after it was accepted in case there were still concerns. The mutex code was added because it was requested in review. IIRC, there were concerns about multiple pass managers registering signal handlers. Since it is for debugging, efficiency of the mutex code is not as important.

Sure, the auto-detection could be used as you suggest, but the control is still needed by the user. Consider what happens when auto-detection says the system supports colour and the user captures the output to a file. They then bring it up in an editor (say vim or emacs) which doesn't recognize the colour characters and show as ^[[31m- and ^[[31m+ (in emacs or vim). By giving the user control over whether they are generated, they can include them if they are examining the results on a screen (or using more) or strip them out when they plan to capture and edit the results (for large changes).

I don't think we want auto-detection because the user needs to be able to control it. If it is just going to the screen, the colour is useful, but if the output is being captured and brought up in emacs/vim/etc, the colour characters just show as control characters. less on LINUX doesn't handle the colour characters; other utilities may also have problems with them.

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.

Rebase to facilitate applying patch. Also, I removed the indenting for a grouping of links as I don't think it was useful.

This will require changes based on changes that have been made to the code it uses. The changes will be made after that code lands.

Updated the code to reflect various changes made in reviews
for the other change printers that have landed. Also added
unit tests.

I think that is a great idea, especially since it is likely it is being captured anyway... Perhaps with -print-on-crash=<filename> with the current behaviour when =<filename> is not specified? As a subsequent change rather than in this one.

Respond to review comments. Minor changes including fixing typos, using function_ref, enum classes and moving code around.

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.