This is an archive of the discontinued LLVM Phabricator instance.

[NewPM][PassInstrument] Add PrintPass callback to StandardInstrumentations
ClosedPublic

Authored by ychen on Jul 28 2020, 10:36 AM.

Details

Summary

Problem:
Right now, our "Running pass" is not accurate when passes are wrapped in adaptor because adaptor is never skipped and a pass could be skipped. The other problem is that "Running pass" for a adaptor is before any "Running pass" of passes/analyses it depends on. (for example, FunctionToLoopPassAdaptor). So the order of printing is not the actual order.

Solution:
Doing things like PassManager::Debuglogging is very intrusive because we need to specify Debuglogging whenever adaptor is created. (Actually, right now we're not specifying Debuglogging for some sub-PassManagers. Check PassBuilder)

This patch move debug logging for pass as a PassInstrument callback. We could be sure that all running passes are logged and in the correct order.

This could also be used to implement hierarchy pass logging in legacy PM. We could also move logging of pass manager to this if we want.

The test fixes looks messy. It includes changes:

  • Remove PassInstrumentationAnalysis
  • Remove PassAdaptor
  • If a PassAdaptor is for a real pass, the pass is added
  • Pass reorder (to the correct order), related to PassAdaptor
  • Add missing passes (due to Debuglogging not passed down)

Diff Detail

Event Timeline

ychen created this revision.Jul 28 2020, 10:36 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 28 2020, 10:36 AM
ychen requested review of this revision.Jul 28 2020, 10:36 AM
ychen edited the summary of this revision. (Show Details)Jul 28 2020, 10:44 AM

Does it make sense to have the option to enable seeing where the adaptors are run?

Otherwise this looks good, it's nice to have the "Starting" and "Finished" match now.

Does it make sense to have the option to enable seeing where the adaptors are run?

Adding a static:cl at the top of StandardInstrumentation.cpp should do it. Do you want me to add it now or add it when we have use cases?

Otherwise this looks good, it's nice to have the "Starting" and "Finished" match now.

asbirlea accepted this revision.Jul 28 2020, 11:14 AM

I'm ok if this is added in either this or a follow-up patch.
I think it's useful to have it in the codebase now, with one test to showcase it (e.g. do another run line in new-pass-manager.ll, with check-prefixes).

This revision is now accepted and ready to land.Jul 28 2020, 11:14 AM
aeubanks accepted this revision.Jul 28 2020, 11:59 AM

Thanks for doing this!

ychen updated this revision to Diff 281395.Jul 28 2020, 4:03 PM
  • Add -debug-pass-manager-verbose to print adaptor passes. (test case in pass-pipeline-parsing.ll where we could ask for adaptor pass from -passes)
ychen updated this revision to Diff 281399.Jul 28 2020, 4:11 PM
  • Update -debug-pass-manager-verbose test. Some output should be ignored.
This revision was landed with ongoing or failed builds.Jul 30 2020, 10:08 AM
This revision was automatically updated to reflect the committed changes.