Common CLI flags registry #990
Conversation
adam-smnk
added
benchmark
|
No benchmark regressions. |
|
@KavithaTipturMadhu This should help with flags through pass bundles. |
| namespace opt { | ||
|
|
||
| // Print MLIR before lowering | ||
| extern llvm::cl::opt<std::string> printMLIR; |
There was a problem hiding this comment.
Wait, that's odd. extern is supposed to be used in a C file when another C file declares your variable, not in a header.
It's also bad form to initialize everything statically before _main. This is why we have things like pass options, etc.
I don't think this is a step forward.
There was a problem hiding this comment.
extern is normally rather redundant as that's header's job to provide declarations. However, explicit extern can be useful to make intention more obvious in certain cases. This seems to be a common pattern for CLI options across LLVM headers.
CLI flags are global object anyway, this doesn't change their initialization or lifetime - only helps with scope. The main idea is to have a central place that has all the flags gathered together and easily accessible by pass bundles. This eliminates the need to create multiple duplicate pass options only to pipe a flag to a single pass nested deep within 5 other bundles.
I agree it shouldn't be misused for controlling individual passes like convert X to Y.
There was a problem hiding this comment.
Perhaps to put this more simply, this header is a collection of available singletons.
There was a problem hiding this comment.
I'm worried we'll leak anyway because not there's no indication where this flag will have an effect.
There was a problem hiding this comment.
On the other hand, currently it takes jumping across multiple files to track down the origin of option's value.
Plus, CLI flags generally define desired values. Now if they have defaults, these values have to be propagated through all pass option defaults. Otherwise running default-pipeline vs other standalone pass can be much different.
I think between the two, this has potentially fewer points of failure and requires less maintenance but ultimately I'm fine with either.
There was a problem hiding this comment.
Well, it's just an experimental alternative - I'm not fully sure it's better.
If you don't feel convinced, then let's leave things as they are.
There was a problem hiding this comment.
the thing is, we should be working to remove the bundles (in favour of schedules) and this churn will not help Kavitha's PR that much anyway. mlir-runner has moved from statically initialized arguments to structure based use to avoid the init cost. I'm not convinced this is a good idea.
|
Not necessarily beneficial - closing. |
|
Note for future reference: there is actually a nice utility |
Moves CLI flags used to control pipelines behavior to a common registry.
This cleanup allows to remove the overhead of piping options through multiple layers of pass bundles. Now, the common CLI flags are globally accessible and can be pulled directly as a dependency by any pass bundle.
Note that individual passes should be self contained and define all necessary options.