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

Chandler Carruth via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 3 00:32:08 PDT 2016


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





More information about the llvm-commits mailing list