Introduce -print-changed=[diff | diff-quiet] which print the changes made by a pass in patch-like form. Introduce base classes that hold a textual represent of the IR based on basic blocks and a base class for comparing this representation. A new change printer is introduced that uses these classes to save and compare representations of the IR before and after each pass. It only reports when changes are made by a pass (similar to -print-changed) except that the changes are shown in a patch-like format with those lines that are removed shown in red prefixed with '-' and those added shown in green with '+'. This functionality was introduced in my tutorial at the 2020 virtual developer's meeting.
Apologies if I'm forgetting things you've said in the past, but what are the other future use cases of this? I'd prefer less templatization when possible.
what about "print-changed-diff"? "print-changes" seems a bit too closed to "print-changed"
probably shouldn't have Linux in the name, this could work on other platforms.
maybe this should be using the APIs in Program.h instead? popen seems a bit too C-like.
return '-line-format' in ld_out
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.
I'm fine with changing the option name. I will update the patch and address the other comments.
Respond to review comments: change option name to print-changed-diff,
change function name and change code to use ExecuteAndWait.
Also add hidden option specifying fully resolved path name to diff to
allow specifying overrides, defaulting to typical linux system path.
There are a lot of unnecessary llvm::, can those be removed?
TBH this IRComparer framework seems over-engineered and it's hard for me to understand all the moving parts and how clients override certain methods, making it hard to review.
Is it possible to make it simpler?
The diff part looks mostly good though.
this part is unnecessary
is a separate method necessary? or can it be done in the constructor?
can you use an enum class? otherwise it'll pollute the llvm namespace
These don't match the names below.
the return value is never used
DiffBinary or DiffBinaryPath?
should this default to the diff on the PATH?
does this work?
looks like StringMap already has an operator==, why recreate it here?
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.
I have addressed your other comments and also made
changes so that the differences are shown in red and
green in addition to being prefixed with '-' and '+', respectively.
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.
just a general comment, I do like the idea of printing diffs between passes, could make looking through pipelines much nicer
seems weird to inherit from StringMap, what about making it a variable instead?
looks like this is only used in the cpp file, move it there?
something containing the word "path" or "binary" seems clearer, like print-changed-diff-binary or print-changed-diff-path
If the temp files are removed at the end, is there any reason for these to be static? Can we just create new temp files every time?
I thought we already had similar utilities for this.
C->begin()->getFunction().getParent(). SCCs cannot be empty.
maybe we can just assume that ChangedIRData has an equality operator? not necessarily in this patch
this won't necessarily be in the same order that the blocks are laid out in the function right? that seems like something we should make sure happens
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.
mostly looks good, a couple nits
function_ref is much lighter weight.
The wording here tripped me up, perhaps The values "diff" and "diff-quiet" will....
can you move these above the StandardInstrumentations code?
I'm wondering if we should just detect any diff in the PATH, but maybe just /usr/bin/diff is fine, idk