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

Wed Aug 3 02:25:06 PDT 2016

As explained (and from Sean), the distinction about transformation is a
good one -- this will actually make the formal specification cleaner.

thanks,

David

On Wed, Aug 3, 2016 at 12:32 AM, Chandler Carruth via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> chandlerc added a comment.
>
> Thanks all, landing momentarily.
>
>
> ================
> Comment at: lib/Passes/PassBuilder.cpp:278
> @@ +277,3 @@
> +static Optional<int> parseRepeatPassName(StringRef Name) {
> +  if (!Name.consume_front("repeat<") || !Name.consume_back(">"))
> +    return None;
> ----------------
> silvas wrote:
> > davidxl wrote:
> > > chandlerc wrote:
> > > > silvas wrote:
> > > > > 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.
> > > > 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.
> > > 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.
> > >
> > > 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)`).
> 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.
>
> ================
> Comment at: test/Other/new-pass-manager.ll:432
> @@ +431,3 @@
> +; RUN:     | FileCheck %s --check-prefix=CHECK-REPEAT-LOOP-PASS
> +; CHECK-REPEAT-LOOP-PASS: Starting llvm::Module pass manager run
> +; CHECK-REPEAT-LOOP-PASS-NEXT: Running pass: ModuleToFunctionPassAdaptor
> ----------------
> silvas wrote:
> > chandlerc wrote:
> > > silvas wrote:
> > > > 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.
> > > 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.)
> > I disagree, but only mildly. If you feel strongly about keeping this
> feel free to commit as-is.
> 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...
>
>
> https://reviews.llvm.org/D22405
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160803/408a164d/attachment.html>