Page MenuHomePhabricator

[NewPM] make parsePassPipeline parse adaptor-wrapped user passes
ClosedPublic

Authored by ychen on Jun 26 2020, 6:10 PM.

Details

Summary

Currently, when parsing text pipeline, different kinds of passes always
introduce nested pass managers. This makes it impossible to test the
adaptor-wrapped user passes from the text pipeline interface which is needed
by D82344 test cases. This also seems useful in general. See comments above
parsePassPipeline.

The syntax would be like mixing passes of different types, but it is
not the same as inferring the correct pass type and then adding the
matching nested pass managers. Strictly speaking, the resulted pipelines
are different.

Diff Detail

Event Timeline

ychen created this revision.Jun 26 2020, 6:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2020, 6:10 PM

I think it's great to have a way to specify the exact nesting of managers we want.
Please extend the changes to all tests.

ychen updated this revision to Diff 274315.Jun 29 2020, 7:56 PM
  • update test

I think this makes sense as long as it's not abused and doesn't make things harder to reason about. Should be fine since most (all?) uses are through opt.

llvm/test/Other/pass-pipeline-parsing.ll
182

what about a separate check for
Running pass: FunctionToLoopPassAdaptor<{{.*}}NoOpLoopPass>
and NoOpLoopPass itself?

Not sure exactly how to write a test for this, but it'd be nice to test a case where we have a module pass and a function pass both named the same. e.g. in PassRegistry.def there are multiple passes called "print".

ychen marked an inline comment as done.Thu, Jul 16, 11:40 AM
ychen added inline comments.
llvm/test/Other/pass-pipeline-parsing.ll
182

I'm happy to do that. What is the motivation?

ychen updated this revision to Diff 278567.Thu, Jul 16, 12:01 PM

Add a test case for 'print' pass in CGSCC pipeline where 'print' should be a function pass.

Not sure exactly how to write a test for this, but it'd be nice to test a case where we have a module pass and a function pass both named the same. e.g. in PassRegistry.def there are multiple passes called "print".

Interesting. I would argue that passes with same name are extremely rare, probably just "print". For "print", it has a version for each pass type except CGSCC which means only CGSCC pipeline would possibly hit this adaptor code path. I've added a test case for this.

aeubanks added inline comments.Thu, Jul 16, 1:18 PM
llvm/test/Other/pass-pipeline-parsing.ll
182

To make sure that the FunctionToLoopPassAdaptor itself isn't skipped for some reason, and the same with the NoOpLoopPass.

ychen marked an inline comment as done.Thu, Jul 16, 2:58 PM
ychen added inline comments.
llvm/test/Other/pass-pipeline-parsing.ll
182

Is line 191 what you refer to?

aeubanks added inline comments.Thu, Jul 16, 3:14 PM
llvm/test/Other/pass-pipeline-parsing.ll
182

no, that one is a different no-op-loop in -passes='module(no-op-function,no-op-loop,no-op-cgscc,cgscc(no-op-function,no-op-loop),function(no-op-loop))'.

line 191 refers to the very last no-op-loop, I meant the first one. basically I think every adaptor "Running pass" message should be checked.
1 for the first no-op-function (module -> function), 2 for the first no-op-loop (module -> function -> loop), 1 for the first no-op-cgscc (module -> cgscc), 1 for the second no-op-function (cgscc -> function), 2 for the second no-op-loop (cgscc -> function -> loop), and 1 for the last no-op-loop (function -> loop)

ychen marked an inline comment as done.Thu, Jul 16, 3:23 PM
ychen added inline comments.
llvm/test/Other/pass-pipeline-parsing.ll
182

First no-op-loop is checked by line 182.

Aren't these covered by the existing Running pass:?

Sorry, I'm still confused.

aeubanks added inline comments.Thu, Jul 16, 4:28 PM
llvm/test/Other/pass-pipeline-parsing.ll
182

Ah I think I see what I'm missing. I was expecting each adaptor pass to print "Running pass:" if it were to run the pass it's adapting, but it doesn't. I think that's a good idea since the adaptor may run but for some reason it might not run its pass (e.g. if it decides to skip it for some reason). WDYT? Maybe that's out of scope for this change.

ychen marked an inline comment as done.Thu, Jul 16, 5:20 PM
ychen added inline comments.
llvm/test/Other/pass-pipeline-parsing.ll
182

I agree. We should fix this o/w, we will see Running pass: for skipped pass. Feel free to submit a patch.

This revision is now accepted and ready to land.Thu, Jul 16, 5:29 PM
asbirlea accepted this revision.Thu, Jul 16, 7:00 PM

Thanks, this is good.

ychen edited the summary of this revision. (Show Details)Sat, Jul 18, 10:25 PM
This revision was automatically updated to reflect the committed changes.