[PATCH] D22724: [PM] Significantly refactor the pass pipeline parsing to be easier to reason about and less error prone.

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.

================
Comment at: lib/Passes/PassBuilder.cpp:561
@@ -401,1 +560,3 @@
+
+  // Now expand the basic registered passes from the .inc file.
 #define FUNCTION_PASS(NAME, CREATE_PASS)                                       \
----------------
silvas wrote:
> Do you need error-checking here to verify that InnerPipeline is empty? Just from staring at the code it seems like `gvn(foo,bar)` will just be silently accepted with the `(foo,bar)` ignored.
> 
> I think down the road it actually would be useful to allow passes to take "arguments". E.g. `inline(150)` or `instcombine(expensivecombines=true)`. That way we can run two inliners in the same pipeline but with different thresholds.
> The instcombine case is actually interesting because currently there is no way to specify an instcombine pass that does not do the "expensive" combines. This means that this textual pipeline description cannot actually describe the O2 pipeline correctly (this is an issue with the old PM too, of course, so it isn't a high priority to fix).
I think we should check that the inner pipeline is empty as you suggest.

I agree that we'll want passes to take arguments. We actually already have that with requires and such.

The design I've been going with is that ()s denote *pipelines*, and <>s denote *arguments*. So, imagining a pass that repeats a pipeline a certain number of times (which I have a review out for but will update based on the new parsing momentarily) we will have "repeat<N>(gvn,whatever)". Does that make sense?


https://reviews.llvm.org/D22724