[PATCH] D86360: Add new hidden option -print-changed which only reports changes to IR

[Jamie Schmeiser via Phabricator via llvm-commits]

jamieschmeiser added a comment.

@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



================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:52
+// also reported.  Other hidden options affect the output from this
+// option.  -filter-passes will limit the output to the named passes
+// that actually change the IR and other passes are reported as filtered out.
----------------
MaskRay wrote:
> The relations with -filter-passes and print-module-scope probably should be moved to a separate graph, because several other `-print-*` (e..g `-print-after-all`) share the documentation.
-filter-passes only supports this option and no other options.  It is new with this code and is described here because it is easier to understand its usage when described in this context.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:306
+    // was there a change in IR?
+    if (same(Before, After))
+      omitAfter(PassID, Name);
----------------
MaskRay wrote:
> same has a trivial definition. Consider deleting the member function
Same is a virtual override and must exist.  See discussion of base class in response to another of your comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86360/new/

https://reviews.llvm.org/D86360