[PATCH] D53895: [LoopUnroll] add parsing for unroll parameters in -passes pipeline

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev marked an inline comment as done.
fedor.sergeev added inline comments.


================
Comment at: lib/Passes/PassBuilder.cpp:1253-1257
+/// This performs customized parsing of pass name with parameters.
+///
+/// We do not need parametrization of passes in textual pipeline very often,
+/// yet on a rare occasion ability to specify parameters right there can be
+/// useful.
----------------
philip.pfaffe wrote:
> fedor.sergeev wrote:
> > philip.pfaffe wrote:
> > > fedor.sergeev wrote:
> > > > chandlerc wrote:
> > > > > Please explain how the parser type parameter works and what is expected of it.
> > > > > 
> > > > > But better yet, how about we just make this routine accept a callable rather than a class, and have this functions return type be the same as that callable?
> > > > > 
> > > > > Something like:
> > > > > ```
> > > > > template <typename ParseCallableT>
> > > > > auto parsePassWithParameters(ParseCallableT Parser, ...) -> decltype(std::decl_ref<ParseCallableT>()("")) {
> > > > >   ...
> > > > > }
> > > > > ```
> > > > > 
> > > > > I suppose you could use result_of or some other trait instead, not sure it'd be any simpler.
> > > > > 
> > > > > The key idea is to make this just a wrapper of another callable which strips off the pass name and the angle brackets.
> > > > > 
> > > > > To handle the empty case, just call the callable with an empty string, and have it return the default value.
> > > > To avoid complications with ParametersT type deduction decided to pass parameters type explicitly into the macro/parsing helper template.
> > > Can you elaborate?
> > ```std::decl_ref<ParseCallableT>()("") ```
> > in Chandler's example above is already not that pretty.
> > And that only gives you Expected<ParameterType>, not ParameterType itself, which I would need in order to produce default ParameterType{} object.
> > Also, I need something to work with at the call site (see where FPM.addPass is being done).
> > Would be nice to have something like Expected<auto>....
> Doesn't necessarily have to be pretty :) Can it be shortened to just `Parser("")` though?
> 
> `Expected<T>::value_type` gives you the T, and at the callsite it can just be `auto`, can it not?
>> To avoid complications with ParametersT type deduction 
> Doesn't necessarily have to be pretty
Well, there should be some merit here :)
Though ::value_type does not make it extra complicated, indeed.
> at the callsite it can just be auto
Given that it is all under macro-expansion, I would like to be quite explicit at the callsite.
auto makes me feel somewhat unsafe there... not sure if there is a real technical argument under that feeling.
Will try it out.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53895/new/

https://reviews.llvm.org/D53895