This is an archive of the discontinued LLVM Phabricator instance.

[NewPM] Support --print-before/after in NPM
ClosedPublic

Authored by aeubanks on Sep 6 2020, 6:38 PM.

Details

Summary
This changes --print-before/after to be a list of strings rather than
legacy passes. (this also has the effect of not showing the entire list
of passes in --help-hidden after --print-before/after, which IMO is
great for making it less verbose).

Currently PrintIRInstrumentation passes the class name rather than pass
name to llvm::shouldPrintBeforePass(), meaning
llvm::shouldPrintBeforePass() never functions as intended in the NPM.
There is no easy way of converting class names to pass names outside of
within an instance of PassBuilder.

This adds a map of pass class names to their short names in
PassRegistry.def within PassInstrumentationCallbacks. It is populated
inside the constructor of PassBuilder, which takes a
PassInstrumentationCallbacks.

Add a pointer to PassInstrumentationCallbacks inside
PrintIRInstrumentation and use the newly created map.

This is a bit hacky, but I can't think of a better way since the short
id to class name only exists within PassRegistry.def. This also doesn't
handle passes not in PassRegistry.def but rather added via
PassBuilder::registerPipelineParsingCallback().

llvm/test/CodeGen/Generic/print-after.ll doesn't seem very useful now
with this change.

Diff Detail

Event Timeline

aeubanks created this revision.Sep 6 2020, 6:38 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 6 2020, 6:38 PM
aeubanks requested review of this revision.Sep 6 2020, 6:38 PM
aeubanks updated this revision to Diff 290159.Sep 6 2020, 6:40 PM

remove some includes

aeubanks updated this revision to Diff 300555.Oct 25 2020, 1:40 PM

move map into PassInstrumentationCallbacks

aeubanks edited the summary of this revision. (Show Details)Oct 25 2020, 1:42 PM
aeubanks added a reviewer: asbirlea.
jamieschmeiser requested changes to this revision.Oct 27 2020, 9:25 AM

The changes are specific to -print-before and -print-after (which is the intended target and this work originated before -print-changed) but could the change be made universal? I would rather see the existing StringRef passed to all the callbacks changed rather than adding the new StringRef to specific callbacks. This way all passes benefit and are consistent. In particular, I would like to see this change applied to -printChanged; this came up in the reviews for that PR. If you would rather not make it universal, then at least change all of the callbacks to also receive the new StringRef.

This revision now requires changes to proceed.Oct 27 2020, 9:25 AM
aeubanks updated this revision to Diff 306307.Nov 18 2020, 9:50 PM

rebase
now we don't add a new parameter to callbacks, instead let individual instrumentations ask PassInstrumentationCallbacks for pass names

aeubanks edited the summary of this revision. (Show Details)Nov 18 2020, 9:50 PM

The changes are specific to -print-before and -print-after (which is the intended target and this work originated before -print-changed) but could the change be made universal? I would rather see the existing StringRef passed to all the callbacks changed rather than adding the new StringRef to specific callbacks. This way all passes benefit and are consistent. In particular, I would like to see this change applied to -printChanged; this came up in the reviews for that PR. If you would rather not make it universal, then at least change all of the callbacks to also receive the new StringRef.

I think this change might be a better way, PTAL

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.

llvm/lib/Passes/PassBuilder.cpp
436

The tests for the options being empty should be moved to a separate function to facilitate expanding this feature for use with other pass instrumentation callbacks. Right now, this is buried in here and if another callback wished to also use this freature, it might be hard to find. Pulling the test out into an aptly named function would make it easier to find and understand.

454

This will silently give no tracing in release mode if the pass name does not exist (ie the error would not be reported and there would be no output). Is this because of efficiency concerns? It will only look for the names that are in the option list, which would typically be empty. Rather than walking the string map, you could have a local stringset, add the pass names into it in the macros above if checking will be done and then use the stringset for the error checking. I think this would be more efficient than walking the stringmap for producing the errors.

aeubanks updated this revision to Diff 308797.Dec 1 2020, 3:22 PM

address review comments

llvm/lib/Passes/PassBuilder.cpp
436

Makes sense, done.

454

You're right, this should normally be empty so it should fine to always check. Removed the check for NDEBUG.

When --print-before/after is not empty, any minor slowdown shouldn't matter since you'd only be using --print-before/after while debugging. So I'd prefer to keep checking the map which is a bit simpler code-wise.

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?

aeubanks updated this revision to Diff 308995.Dec 2 2020, 9:29 AM

fix capitalization
remove extra declaration and fixup includes

aeubanks updated this revision to Diff 309000.Dec 2 2020, 9:45 AM

clean up some more unnecessary llvm::
add comments to function declarations in PrintPasses.h

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?

Fixed the capitalization.

Do you mean moving around options/definitions into PrintPasses.cpp? The declarations/definitions don't really have a good place to live right now, I moved them to their own file to make it cleaner.

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.

llvm/include/llvm/IR/PassInstrumentation.h
126

Comments describing these functions?

aeubanks updated this revision to Diff 309052.Dec 2 2020, 2:05 PM

add comments to new methods

This revision is now accepted and ready to land.Dec 3 2020, 6:25 AM
ychen accepted this revision.Dec 3 2020, 10:04 AM

It is very unfortunate that we have to manage and translate between two sets of names (one pass name and one type name). This makes me wonder if we just keep the pass name as the return value of PassInfoMixin::name and get rid of class name everywhere. Right now I couldn't think of anything is blocking us from doing that. WDYT? @asbirlea ?

llvm/include/llvm/Passes/StandardInstrumentations.h
24

It is better if forward declaring PassInstrumentationCallbacks could remove this include.

It is very unfortunate that we have to manage and translate between two sets of names (one pass name and one type name). This makes me wonder if we just keep the pass name as the return value of PassInfoMixin::name and get rid of class name everywhere. Right now I couldn't think of anything is blocking us from doing that. WDYT? @asbirlea ?

We'd have to move the names from PassRegistry.def to every pass class definition which would be a lot of work but definitely feasible.

ychen added a comment.Dec 3 2020, 11:54 AM

It is very unfortunate that we have to manage and translate between two sets of names (one pass name and one type name). This makes me wonder if we just keep the pass name as the return value of PassInfoMixin::name and get rid of class name everywhere. Right now I couldn't think of anything is blocking us from doing that. WDYT? @asbirlea ?

We'd have to move the names from PassRegistry.def to every pass class definition which would be a lot of work but definitely feasible.

That's true. The issue of translation also happens for codegen using NPM where both target-dependent and target-independent passes need the translation. Looking at my prototype, there is a lot of boilerplate for that. I think the one-time cost of moving names around should be worthwhile.

aeubanks updated this revision to Diff 309338.Dec 3 2020, 12:42 PM

forward declare class

aeubanks marked an inline comment as done.Dec 3 2020, 12:43 PM
This revision was landed with ongoing or failed builds.Dec 3 2020, 5:03 PM
This revision was automatically updated to reflect the committed changes.
hoy added a subscriber: hoy.Jan 7 2021, 6:14 PM
hoy added inline comments.
llvm/lib/Passes/PassBuilder.cpp
459

@aeubanks This seems not working with MIR passes under newpm. Would it make sense to issue a warning instead to unblock that? Thanks.

aeubanks added inline comments.Jan 7 2021, 7:22 PM
llvm/lib/Passes/PassBuilder.cpp
459

That's a good point, I'll do that.