This is an archive of the discontinued LLVM Phabricator instance.

[LoopUnroll] add parsing for unroll parameters in -passes pipeline
AbandonedPublic

Authored by fedor.sergeev on Oct 30 2018, 2:12 PM.

Details

Summary

Allow to specify loop-unrolling with optional parameters explicitly
spelled out in -passes pipeline specification.
Introducing somewhat generic way of specifying parameters parsing via
FUNCTION_PASS_PARAMETRIZED pass registration.

Syntax of parametrized unroll pass name is as follows:

'unroll<' parameter-list '>'

Where parameter-list is ';'-separate list of parameter names and optlevel:

optlevel: 'O[0-3]'
parameter: { 'partial' | 'peeling' | 'runtime' | 'upperbound' }
negated:  'no-' parameter

Example:

-passes=loop(unroll<O3;runtime;no-upperbound>)

 this invokes LoopUnrollPass configured with OptLevel=3,
 Runtime, no UpperBound, everything else by default.

Diff Detail

Event Timeline

fedor.sergeev created this revision.Oct 30 2018, 2:12 PM
fedor.sergeev added a comment.EditedOct 30 2018, 2:19 PM

Note: had to use ';' as a delimiter, since using ',' as a delimiter will require doing more sophisticated parsing of a pipeline text (would no longer be able to just split on ',' in parsePipelineTest).

fedor.sergeev edited the summary of this revision. (Show Details)Oct 31 2018, 1:50 AM

Curious how others feel, but I'd prefer to keep all the parsing in the pass builder rather than start calling back into the passes. I think that keeps the passes more minimal, and I don't think this kind of parsing should he so widespread as to need separation. If a few passes have custom logicbib pass builder that seems ok...

Curious how others feel, but I'd prefer to keep all the parsing in the pass builder rather than start calling back into the passes. I think that keeps the passes more minimal, and I don't think this kind of parsing should he so widespread as to need separation. If a few passes have custom logic in pass builder that seems ok...

Sure thing I can keep the implementation of parser local to pass builder.
It needs to have a mapping for unroll parameter names, which does not really belong to pass builder,
but then it is a rather simple logic so there should not be any real harm...

What is the advantage of this over using command line args?

Obviously you can do per-pass-instance configuration, but is that ever necessary in parsed pipelines?

What is the advantage of this over using command line args?

Obviously you can do per-pass-instance configuration, but is that ever necessary in parsed pipelines?

I think there are a few passes where we routinely have different variations at different stages of the pipeline in common pipelines people are building / experimenting with.

I certainly hope we won't need this level of flexibility often or it will need a somewhat different engineering approach, but as long as it is fairly limited to <10 or small 10s of passes, I can understand the desire.

Sure thing I can keep the implementation of parser local to pass builder.
It needs to have a mapping for unroll parameter names, which does not really belong to pass builder,
but then it is a rather simple logic so there should not be any real harm...

FWIW, I really would have the whole parse thing to build the LoopUnrollOptions from a string to live exclusively inside the PassBuilder implementation.

The reason is that I'd rather not widely advertise a string-parsing API for building this. I really want to keep code-based users *not* parsing strings. ;]

What is the advantage of this over using command line args?
Obviously you can do per-pass-instance configuration, but is that ever necessary in parsed pipelines?

This comes as a followup to D53440, where I needed to test LoopUnrollOptions change and hence a way to specify a single pass instance having arguments different from a global configuration.
I can also imagine this being useful during bug triaging in some corner cases - our downstream pipeline uses different flavors of loop-unroll routinely.

FWIW, I really would have the whole parse thing to build the LoopUnrollOptions from a string to live exclusively inside the PassBuilder implementation.

I do hear ya! Had no time to work on it, crazy end of the week here...

fedor.sergeev planned changes to this revision.Nov 21 2018, 11:10 PM

finally, moved parameters parsing from LoopUnrollPass into PassBuilder,
addressing @chandlerc's comments

chandlerc requested changes to this revision.Dec 23 2018, 1:58 AM

Awesome, comments inline!

lib/Passes/PassBuilder.cpp
1232

As Name is a by-value parameter, this should just be starts_with and ends_with?

1237–1241

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.

1249

Please use prose for comments. This comemnt doesn't help the reader much IMO, but the prose I think you intended would:

// If no explicit parameters are specified, simply return a default constructed object.
lib/Passes/PassRegistry.def
145–147

Separate the parameterized X-macros into a separate block? Seems a bit cleaner, they won't sort nicely intermingled.

This revision now requires changes to proceed.Dec 23 2018, 1:58 AM

addressing comments

fedor.sergeev marked 5 inline comments as done.Dec 24 2018, 5:13 AM
fedor.sergeev added inline comments.
lib/Passes/PassBuilder.cpp
1237–1241

To avoid complications with ParametersT type deduction decided to pass parameters type explicitly into the macro/parsing helper template.

fedor.sergeev marked an inline comment as done.Dec 24 2018, 5:37 AM
philip.pfaffe added inline comments.Jan 3 2019, 3:09 AM
lib/Passes/PassBuilder.cpp
1237–1241

Can you elaborate?

fedor.sergeev marked an inline comment as done.Jan 3 2019, 7:41 AM
fedor.sergeev added inline comments.
lib/Passes/PassBuilder.cpp
1237–1241
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>....

philip.pfaffe added inline comments.Jan 3 2019, 7:46 AM
lib/Passes/PassBuilder.cpp
1237–1241

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?

fedor.sergeev marked an inline comment as done.Jan 3 2019, 12:51 PM
fedor.sergeev added inline comments.
lib/Passes/PassBuilder.cpp
1237–1241

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.

okey, now without explicit ParametersT template parameter.

Cool! Down to bikeshed comments :)

lib/Passes/PassRegistry.def
233

I'd prefer PARAMETERIZED_FUNCTION_PASS or FUNCTION_PASS_WITH_PARAMS or the like.

236

No \ required here.

fedor.sergeev marked 2 inline comments as done.Jan 6 2019, 10:42 AM
philip.pfaffe accepted this revision.Jan 10 2019, 12:44 AM

Looks good to me, thanks!

fedor.sergeev abandoned this revision.Jan 11 2019, 5:10 AM

This has been pushed as https://reviews.llvm.org/rL350808, just neglected to put a reference to this review, so it did not auto-close.