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

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe 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.
----------------
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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53895