Page MenuHomePhabricator

Introduce -print-changed=[diff | diff-quiet] which show changes in patch-like format
ClosedPublic

Authored by jamieschmeiser on Nov 20 2020, 12:51 PM.

Details

Summary
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.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2020, 12:51 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
jamieschmeiser requested review of this revision.Nov 20 2020, 12:51 PM

Changes based on clang-tidy/format

aeubanks added inline comments.Nov 30 2020, 12:05 PM
llvm/include/llvm/Passes/StandardInstrumentations.h
274

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.

llvm/lib/Passes/StandardInstrumentations.cpp
94

what about "print-changed-diff"? "print-changes" seems a bit too closed to "print-changed"

103

probably shouldn't have Linux in the name, this could work on other platforms.

139

maybe this should be using the APIs in Program.h instead? popen seems a bit too C-like.

llvm/test/Other/ChangePrinters/lit.local.cfg
13–16

return '-line-format' in ld_out

Also, could you say in the description that this shells out to diff?

jamieschmeiser edited the summary of this revision. (Show Details)Dec 1 2020, 9:34 AM

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.

llvm/include/llvm/Passes/StandardInstrumentations.h
252

this part is unnecessary

277

is a separate method necessary? or can it be done in the constructor?

331
348

can you use an enum class? otherwise it'll pollute the llvm namespace

362–363

These don't match the names below.
Also would it be simpler to just allow each subclass to handle Function vs Module, rather than having virtual methods for each one?

366

the return value is never used

llvm/lib/Passes/StandardInstrumentations.cpp
98

DiffBinary or DiffBinaryPath?

99

should this default to the diff on the PATH?

432

does this work?

458

looks like StringMap already has an operator==, why recreate it here?

jamieschmeiser edited the summary of this revision. (Show Details)

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.

mamai added a subscriber: mamai.Dec 16 2020, 11:40 AM

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 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.

That makes a lot of sense to me.

jamieschmeiser retitled this revision from Introduce -print-changes which show changes in patch-like format to Introduce -print-changed=[diff | diff-quiet] which show changes in patch-like format.
jamieschmeiser edited the summary of this revision. (Show Details)

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.

just a general comment, I do like the idea of printing diffs between passes, could make looking through pipelines much nicer

llvm/include/llvm/Passes/StandardInstrumentations.h
277

seems weird to inherit from StringMap, what about making it a variable instead?

290

looks like this is only used in the cpp file, move it there?

llvm/lib/Passes/StandardInstrumentations.cpp
97

diff

100

something containing the word "path" or "binary" seems clearer, like print-changed-diff-binary or print-changed-diff-path

110–111

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?

458

ping
(and if you make ChangedFuncData contain a StringMap as a variable you can just compare the two)

622

I thought we already had similar utilities for this.
Should probably be a static function instead of part of ChangedIRComparer.

627

C->begin()->getFunction().getParent(). SCCs cannot be empty.

1067

maybe we can just assume that ChangedIRData has an equality operator? not necessarily in this patch

1083

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

llvm/include/llvm/Passes/StandardInstrumentations.h
295
300

function_ref is much lighter weight.

llvm/lib/Passes/StandardInstrumentations.cpp
65–66

The wording here tripped me up, perhaps The values "diff" and "diff-quiet" will....

74

enum class?

588
1051–1098

can you move these above the StandardInstrumentations code?

llvm/test/Other/ChangePrinters/lit.local.cfg
6

I'm wondering if we should just detect any diff in the PATH, but maybe just /usr/bin/diff is fine, idk

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

aeubanks accepted this revision.Jan 26 2021, 11:16 AM
This revision is now accepted and ready to land.Jan 26 2021, 11:16 AM