This is an archive of the discontinued LLVM Phabricator instance.

[NewPM] Print 'Skipping pass' as pass instrumentation
ClosedPublic

Authored by aeubanks on Aug 6 2020, 7:52 PM.

Details

Summary

If OptNoneInstrumentation prints it instead, 'Skipping pass' will print for even required passes.

Diff Detail

Event Timeline

aeubanks created this revision.Aug 6 2020, 7:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2020, 7:52 PM
aeubanks requested review of this revision.Aug 6 2020, 7:52 PM
ychen added a comment.Aug 7 2020, 9:01 AM

Looks good. Please add a test case.

ychen added inline comments.Aug 7 2020, 12:54 PM
llvm/lib/Passes/StandardInstrumentations.cpp
314

We probabaly don't need to check adaptor&pass manager here because they are never skipped.

317

It would be more clear to also print the IR name as registerBeforeNonSkippedPassCallback did (using unwrapAndPrint) .

llvm/test/Feature/optnone-opt.ll
95 ↗(On Diff #283951)

Yeah, we don't have a required user pass yet to test. And I would rather not special-casing Verifer pass for this. How about, instead of doing this, extending NPM-LOOP check with IR names so we know the new code path is covered. We could defer the required passes testing for the future. There are some candidate machine passes that could be marked required.

aeubanks updated this revision to Diff 284018.Aug 7 2020, 1:28 PM

Print IR name

aeubanks marked an inline comment as done.Aug 7 2020, 1:28 PM
aeubanks added inline comments.
llvm/lib/Passes/StandardInstrumentations.cpp
314

I could imagine them being skipped in the future. For example, an adaptor could check if the pass it's wrapping is required or not. So I think keeping this is fine.

llvm/test/Feature/optnone-opt.ll
95 ↗(On Diff #283951)

The point of this patch was that "Skipping pass" was printed when it shouldn't have been. Adding checks to NPM-LOOP doesn't really test that, CHECK-NOT: Skipping pass is really the important thing, and for that we need some required pass.
I don't see what's wrong with making VerifierPass required in this patch as opposed to a patch that comes right after this.
I noticed this as I was writing a test for making a bunch of passes required in preparation for turning on -enable-npm-optnone by default.

ychen added inline comments.Aug 7 2020, 1:56 PM
llvm/lib/Passes/StandardInstrumentations.cpp
314

If it is for the future and you're worried that it may not work as expected. I would suggest adding an assert to check the pass could not be special pass yet. Doing so we don't need to add a test for this and worry about the use cases for it.

llvm/test/Feature/optnone-opt.ll
95 ↗(On Diff #283951)

Then let's check pass managers or adaptors as the required pass (checking there are no Skipping pass for them or using CHECK-NEXT). In terms of code path, they make no difference from the required user pass. Making Verifier pass required could be a separate patch because we need to discuss the use cases for that.

aeubanks updated this revision to Diff 284065.Aug 7 2020, 2:51 PM

Check pass manager instead of verifier

aeubanks marked an inline comment as done.Aug 7 2020, 2:52 PM
aeubanks added inline comments.
llvm/test/Feature/optnone-opt.ll
95 ↗(On Diff #283951)

Sounds good, done.

ychen added a comment.Aug 7 2020, 2:54 PM

LGTM. Thanks!

ychen accepted this revision.Aug 7 2020, 2:55 PM
This revision is now accepted and ready to land.Aug 7 2020, 2:55 PM
This revision was landed with ongoing or failed builds.Aug 7 2020, 3:02 PM
This revision was automatically updated to reflect the committed changes.