This is an archive of the discontinued LLVM Phabricator instance.

[PM] Add a generic 'repeat N times' pass wrapper to the new pass manager.
ClosedPublic

Authored by chandlerc on Jul 15 2016, 3:24 AM.

Details

Summary

While this has some utility for debugging and testing on its own, it is
primarily intended to demonstrate the technique for adding custom
wrappers that can provide more interesting interation behavior in
a nice, orthogonal, and composable layer.

Being able to write these kinds of very dynamic and customized controls
for running passes was one of the motivating use cases of the new pass
manager design, and this gives a hint at how they might look. The actual
logic is tiny here, and most of this is just wiring in the pipeline
parsing so that this can be widely used.

I'm adding this now to show the wiring without a lot of business logic.
This is a precursor patch for showing how a "iterate up to N times as
long as we devirtualize a call" utility can be added as a separable and
composable component along side the CGSCC pass management.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc updated this revision to Diff 64117.Jul 15 2016, 3:24 AM
chandlerc retitled this revision from to [PM] Add a generic 'repeat N times' pass wrapper to the new pass manager..
chandlerc updated this object.
chandlerc added a subscriber: llvm-commits.
chandlerc updated this revision to Diff 66605.Aug 2 2016, 6:48 PM

Update to be based on the cleaned up parsing in https://reviews.llvm.org/D22724

hfinkel accepted this revision.Aug 2 2016, 7:16 PM
hfinkel added a reviewer: hfinkel.
hfinkel added a subscriber: hfinkel.

Nice example ;)

LGTM

This revision is now accepted and ready to land.Aug 2 2016, 7:16 PM
silvas added a subscriber: silvas.Aug 2 2016, 7:27 PM
silvas added inline comments.
lib/Passes/PassBuilder.cpp
278 ↗(On Diff #66605)

It worries me that we're going down the path of "no proper lexer". D22724 improved the state of things somewhat.

I just have scars. Every time I've decided to not put a proper lexer or tokenization step in front of any kind of "parsing" I have always regretted it down the road.

It would be great if you could improve this in future patches (this would also probably make source location reporting much easier too since the token can store a source location).

Just an FYI. I think this is fine for now.

test/Other/new-pass-manager.ll
432 ↗(On Diff #66605)

Please don't just blindly check the exact output. Check for what you are looking for (e.g. a particular pass being run a particular number of times).
Same for the other tests.

chandlerc added inline comments.Aug 2 2016, 7:34 PM
lib/Passes/PassBuilder.cpp
278 ↗(On Diff #66605)

Yea, I don't completely disagree and expect at some point this will become more formalized. I'm mostly trying to follow a lazy design principle so that we don't over-engineer this early on because I'm not yet sure what some of the constraints will be. As things settle in terms of functionality, it'll probably become more worthwhile to then harden this properly.

test/Other/new-pass-manager.ll
432 ↗(On Diff #66605)

I'm not just blindly checking the full output.

At several points, I had bugs with too many intermediate pass managers. Having the precise output checked made it really obvious.

While I don't think we should have very many places that check the *exact* pipeline construction details in this way, I think it is reasonable to have roughly one place that surfaces any change in the pipeline structure so that the structure doesn't change in unintended ways. I could reduce the repeatition count to 2 to make this a bit less chatty if that would help, but not sure that it would.

(It is a bit unfortunate that I have to check the *analysis* runs here, and I actually thought about how to avoid that and didn't come up with anything better. It would end up being giant hunks of -NOT checks to blacklist everything that shouldn't go in the space the analysis runs did, and would still end up hard coding the analysis runs go in that space. This is just a fundamental problem with FileCheck based tests -- when you want the test to fail if *new* things show up, you end up hard coding more than you wanted.)

davidxl added a subscriber: davidxl.Aug 2 2016, 9:49 PM
davidxl added inline comments.
lib/Passes/PassBuilder.cpp
278 ↗(On Diff #66605)

I agree with Sean. It would be useful to document the grammar of the pass specification language. This is of course a future item, but doing it earlier allows identifying inconsistency earlier.

It seems to me the grammar is:

<pass_pipeline> := <pass_specification>, <pass_pipeline> | <pass_specification>
<pass_specification>:= <pass_description>|<pre-requisite>| <post_invalidation>
<pass_description> := <irunit>(<pass_pipeline>) | <pass_name>
<irunit> := module | cgscc | function | loop
<pre_requisite> := require< <pass_pipeline> >
<post_invalidation> := invalidate< <pass_pipeline> >

Question: for consistency reason, why using '< >' for require/invalidate' ? using <> for 'repeat<..>' seems making sense.

Also it seems more consistent to make aa-pipeline to adopt similar syntax as 'aa-pipeline(basic_aa, scev)' instead of the current form that uses '=' and it can be specified as part of the passes specification;

-passes=aa-pipeline(basicaa, ...),module(function(jump_threading), globaldce)

Also some of the pass names are allowed to use '< >' in their names such as printer passes -- it should be disallowed.

silvas added inline comments.Aug 3 2016, 12:12 AM
test/Other/new-pass-manager.ll
432 ↗(On Diff #66605)

I disagree, but only mildly. If you feel strongly about keeping this feel free to commit as-is.

silvas added inline comments.Aug 3 2016, 12:16 AM
lib/Passes/PassBuilder.cpp
278 ↗(On Diff #66605)

Also it seems more consistent to make aa-pipeline to adopt similar syntax as 'aa-pipeline(basic_aa, scev)' instead of the current form that uses '=' and it can be specified as part of the passes specification;

-passes=aa-pipeline(basicaa, ...),module(function(jump_threading), globaldce)

This is a bit inconsistent, because aa-pipeline(basicaa, ...) doesn't create a transformation. Notice that all other items in the -passes= argument create transformations (even printer passes have the PreservedAnalyses run(IRUnitT &IR, AnalysisManager &AM) signature). aa-pipeline configures something for the overall pipeline, and it is the only such configuration aspect currently (actually, -debug-pass-manager could be considered to be "configuration" as well).

That is, the intent is that <pass_specification> production currently always expands to something that is a transformation (has signature PreservedAnalyses run(IRUnitT &IR, AnalysisManager &AM)).

Thanks all, landing momentarily.

lib/Passes/PassBuilder.cpp
278 ↗(On Diff #66605)

As Sean said, the AA stuff is really different.

The reason there is an '=' in there is because the AA pipeline is actually a separate commandline flag. So it is a separate parsed entity, and its grammar is just a comma separated list of names.

Regarding <>s, I think there is a consistent set of passes that accept some parameter and use <>s for that parameter. The "require" or "print" passes accept a parameter indicating what is required or what is printed (resp.). The repeat pass accepts an integer for the number of repeatitions. The ()s are different -- in every case currently (I think) they form a pass manager populated by passing a nested pipeline.

Still, this would certainly (eventually) benefit from formalization. I'd like to have the infrastructure in place first so that it can reflect all the variations we end up needing at least for the initial cut. I have one particular piece of infrastructure queued up that may influence this syntax depending on which design we take.

But none of this is really for this patch. =D So we should resume this discussion when things have settled a bit and someone has time to push this particular aspect forward.

test/Other/new-pass-manager.ll
432 ↗(On Diff #66605)

Thanks. This specific case showed me a bug that was obviated by the cleaner parsing logic in an earlier iteration of this patch, so I'd like to keep it. But I am 100% sympathetic to the concern over brittle tests here, and I'll keep thinking about if there is a better way to make changes in the exact pass structure (that may be unintended) visible without making it a burden for future maintainers. So far, comments and a "golden test" style test is the best idea I've got but that doesn't make it especially good...

This revision was automatically updated to reflect the committed changes.