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

[David Li via llvm-commits]

davidxl added a subscriber: davidxl.

================
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;
----------------
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. 



https://reviews.llvm.org/D22405