This is an archive of the discontinued LLVM Phabricator instance.

[NFC intended] Refactor code for print-changed to facilitate reuse.
ClosedPublic

Authored by jamieschmeiser on Sep 1 2020, 8:39 PM.

Details

Summary
[NFC intended] Refactor the code for printChanged for reuse and to facilitate
subsequent reporters of changes to the IR in the new pass manager.

Create abstract template base classes for common functionality and give
classes more appropriate names.  The base classes handle all of the
determination of when a function or pass is "interesting" and should be
reported or filtered out. They have pure virtual functions which are called
when a change by a pass has been recognized so the derived class need only
provide the overrides to present the information about the changing IR.
There are at least 2 more change reporters to come (which were presented
in my tutorial at the 2020 llvm developer's meeting) that derive from
these classes.

Respond to review comments: move function out of line, remove inline keyword,

remove unneeded qualifiers, simplify comparison.

Diff Detail

Event Timeline

jamieschmeiser created this revision.Sep 1 2020, 8:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2020, 8:39 PM
jamieschmeiser requested review of this revision.Sep 1 2020, 8:39 PM

Note that this code makes use of the linux diff function to determine what has changed in the IR. This will likely require altering for other platforms, which is a future enhancement. The testing may require attention before this PR can land and any suggestions on how to handle this would be appreciated.

Rebase to pick up review changes for https://reviews.llvm.org/D86360 and
alter this code to match those changes (reordered parameters, added const
to parameters)

This is an update based on changes that have been made to the code in the review
for https://reviews.llvm.org/D86360. I have moved much of the code and templates into
a new header and body files as it is becoming more complex. I have also changed
some of the class names from being "change printer" based to "change reporter"
based because it doesn't imply how the changes are shown (ie, the changes do not
necessarily need to be printed).

Move enum out of template, make space between end template delimiters
optional in new test.

madhur13490 added inline comments.Sep 8 2020, 2:05 AM
llvm/include/llvm/IR/ChangeReporters.h
258 ↗(On Diff #290040)

Typo: compare

llvm/lib/IR/ChangeReporters.cpp
150 ↗(On Diff #290040)

You are not using this variable.

187 ↗(On Diff #290040)

can combine above two "if"s with logical or and return false.

llvm/lib/Passes/StandardInstrumentations.cpp
115

Why not use standard C++ file handling? This is old C's way of manipulating files.

139

I can be unsigned.

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

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.

Respond to review comments on this PR: various requested changes.
Remove change-printer.ll because tests are now in change-reporters.ll

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

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.

Matt added a subscriber: Matt.Oct 5 2020, 12:08 PM

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.

Removed unnecessary template parameter and improved testing.

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.

This is a really big change and it looks like there are multiple things happening: some refactoring and the actual diff printer, could you split this up so it's more manageable to review?

jamieschmeiser retitled this revision from Add a new hidden option -print-changes that reports in a patch-like notation when a pass actually changes the IR. to [NFC intended] Refactor code for print-changed to facilitate reuse..
jamieschmeiser edited the summary of this revision. (Show Details)

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.

lg aside from some nits

llvm/include/llvm/Passes/StandardInstrumentations.h
91

can you move the definition to the .cpp file? Also, the llvm:: namespace shouldn't be necessary (throughout this entire patch)

108

are the inlines necessary?

llvm/lib/Passes/StandardInstrumentations.cpp
225

Before == After seems simpler

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

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

aeubanks accepted this revision.Nov 9 2020, 9:23 AM
This revision is now accepted and ready to land.Nov 9 2020, 9:23 AM
aeubanks added inline comments.Nov 9 2020, 9:46 AM
llvm/lib/Passes/StandardInstrumentations.cpp
191–192

I think I prefer Name.empty(), but up to you

Rebasing code on latest master before delivering