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

[fedor.sergeev]

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.

[fedor.sergeev]

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).

[chandlerc]

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...

[fedor.sergeev]

In D53895#1281931, @chandlerc wrote:

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...

[fedor.sergeev]

rebased

[philip.pfaffe]

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?

[chandlerc]

In D53895#1285582, @philip.pfaffe wrote:

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.

In D53895#1281939, @fedor.sergeev wrote:

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. ;]

[fedor.sergeev]

In D53895#1285582, @philip.pfaffe wrote:

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.

[fedor.sergeev]

In D53895#1285591, @chandlerc wrote:

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]

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

[chandlerc]

Awesome, comments inline!

lib/Passes/PassBuilder.cpp

1248

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

1253–1257

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.

1265

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

148–150

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

[fedor.sergeev]

addressing comments

[fedor.sergeev]

lib/Passes/PassBuilder.cpp

1253–1257

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

[fedor.sergeev]

ping?

[philip.pfaffe]

lib/Passes/PassBuilder.cpp

1253–1257

Can you elaborate?

[fedor.sergeev]

lib/Passes/PassBuilder.cpp

1253–1257

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]

lib/Passes/PassBuilder.cpp

1253–1257

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]

lib/Passes/PassBuilder.cpp

1253–1257

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.

[fedor.sergeev]

okey, now without explicit ParametersT template parameter.

[philip.pfaffe]

Cool! Down to bikeshed comments :)

lib/Passes/PassRegistry.def

241

I'd prefer PARAMETERIZED_FUNCTION_PASS or FUNCTION_PASS_WITH_PARAMS or the like.

244

No \ required here.

[fedor.sergeev]

cleanup

[fedor.sergeev]

ping?

[philip.pfaffe]

Looks good to me, thanks!

[fedor.sergeev]

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.

lib/Passes/PassBuilder.cpp

static Optional<int> parseDevirtPassName(StringRef Name) {
   if (!Name.consume_front("devirt<") || !Name.consume_back(">"))
     return None;
   int Count;
   if (Name.getAsInteger(0, Count) || Count <= 0)
     return None;
   return Count;
 }
 
+static bool checkParametrizedPassName(StringRef Name, StringRef PassName) {
+  if (!Name.consume_front(PassName))
+    return false;
+  // normal pass name w/o parameters == default parameters
+  if (Name.empty())
+    return true;
+  return Name.consume_front("<") && Name.consume_back(">");
+}
+
+namespace {
+
+/// 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.
+template <typename ParserT>
+Expected<typename ParserT::ParametersT>
+parsePassParameters(ParserT &&Parser, StringRef Name, StringRef PassName) {
+  StringRef Params = Name;
+  if (!Params.consume_front(PassName)) {
+    assert(false && "unexpected failure to parse parametrized pass name");
+  }
+  // default parameters
+  if (Params.empty())
+    return typename ParserT::ParametersT{};
+
+  if (!Params.consume_front("<") || !Params.consume_back(">")) {
+    assert(false && "invalid format for parametrized pass name");
+  }
+
+  if (Error Err = Parser.parse(Params)) {
+    assert(Err.isA<StringError>() &&
+           "Pass parameter parser can only return StringErrors.");
+    return Err;
+  }
+  return Parser.getParameters();
+}
+
+/// Parser of parameters for LoopUnroll pass.
+struct LoopUnrollOptionsParser {
+  using ParametersT = LoopUnrollOptions;
+
+  LoopUnrollOptions UnrollOpts;
+
+  ParametersT getParameters() { return UnrollOpts; }
+  Error parse(StringRef);
+};
+
+Error LoopUnrollOptionsParser::parse(StringRef Params) {
+  while (!Params.empty()) {
+    StringRef ParamName;
+    std::tie(ParamName, Params) = Params.split(';');
+    int OptLevel = StringSwitch<int>(ParamName)
+                       .Case("O0", 0)
+                       .Case("O1", 1)
+                       .Case("O2", 2)
+                       .Case("O3", 3)
+                       .Default(-1);
+    if (OptLevel >= 0) {
+      UnrollOpts.setOptLevel(OptLevel);
+      continue;
+    }
+
+    bool Enable = !ParamName.consume_front("no-");
+    if (ParamName == "partial") {
+      UnrollOpts.setPartial(Enable);
+    } else if (ParamName == "peeling") {
+      UnrollOpts.setPeeling(Enable);
+    } else if (ParamName == "runtime") {
+      UnrollOpts.setRuntime(Enable);
+    } else if (ParamName == "upperbound") {
+      UnrollOpts.setUpperBound(Enable);
+    } else {
+      return make_error<StringError>(
+          formatv("invalid LoopUnrollPass parameter '{0}' ", ParamName).str(),
+          inconvertibleErrorCode());
+    }
+  }
+  return Error::success();
+}
+
+} // namespace
+
 /// Tests whether a pass name starts with a valid prefix for a default pipeline
 /// alias.
 static bool startsWithDefaultPipelineAliasPrefix(StringRef Name) {
   return Name.startswith("default") || Name.startswith("thinlto") ||
          Name.startswith("lto");
 }
 
 /// Tests whether registered callbacks will accept a given pass name.
static bool isFunctionPassName(StringRef Name, CallbacksT &Callbacks) {
 
   // Explicitly handle custom-parsed pass names.
   if (parseRepeatPassName(Name))
     return true;
 
 #define FUNCTION_PASS(NAME, CREATE_PASS)                                       \
   if (Name == NAME)                                                            \
     return true;
+#define FUNCTION_PASS_PARAMETRIZED(NAME, CREATE_PASS, PARSER)                  \
+  if (checkParametrizedPassName(Name, NAME))                                   \
+    return true;
 #define FUNCTION_ANALYSIS(NAME, CREATE_PASS)                                   \
   if (Name == "require<" NAME ">" || Name == "invalidate<" NAME ">")           \
     return true;
 #include "PassRegistry.def"
 
   return callbacksAcceptPassName<FunctionPassManager>(Name, Callbacks);
 }
 
Error PassBuilder::parseFunctionPass(FunctionPassManager &FPM,
   }
 
 // Now expand the basic registered passes from the .inc file.
 #define FUNCTION_PASS(NAME, CREATE_PASS)                                       \
   if (Name == NAME) {                                                          \
     FPM.addPass(CREATE_PASS);                                                  \
     return Error::success();                                                   \
   }
+#define FUNCTION_PASS_PARAMETRIZED(NAME, CREATE_PASS, PARSER)                  \
+  if (checkParametrizedPassName(Name, NAME)) {                                 \
+    auto Params = parsePassParameters(PARSER, Name, NAME);                     \
+    if (!Params)                                                               \
+      return Params.takeError();                                               \
+    FPM.addPass(CREATE_PASS(Params.get()));                                    \
+    return Error::success();                                                   \
+  }
 #define FUNCTION_ANALYSIS(NAME, CREATE_PASS)                                   \
   if (Name == "require<" NAME ">") {                                           \
     FPM.addPass(                                                               \
         RequireAnalysisPass<                                                   \
             std::remove_reference<decltype(CREATE_PASS)>::type, Function>());  \
     return Error::success();                                                   \
   }                                                                            \
   if (Name == "invalidate<" NAME ">") {                                        \

lib/Passes/PassRegistry.def

 FUNCTION_ALIAS_ANALYSIS("scoped-noalias-aa", ScopedNoAliasAA())
 FUNCTION_ALIAS_ANALYSIS("type-based-aa", TypeBasedAA())
 #undef FUNCTION_ALIAS_ANALYSIS
 #undef FUNCTION_ANALYSIS
 
 #ifndef FUNCTION_PASS
 #define FUNCTION_PASS(NAME, CREATE_PASS)
 #endif
+#ifndef FUNCTION_PASS_PARAMETRIZED
+#define FUNCTION_PASS_PARAMETRIZED(NAME, CREATE_PASS, PARSER)
+#endif
 FUNCTION_PASS("aa-eval", AAEvaluator())
 FUNCTION_PASS("adce", ADCEPass())
 FUNCTION_PASS("add-discriminators", AddDiscriminatorsPass())
 FUNCTION_PASS("aggressive-instcombine", AggressiveInstCombinePass())
 FUNCTION_PASS("alignment-from-assumptions", AlignmentFromAssumptionsPass())
 FUNCTION_PASS("bdce", BDCEPass())
 FUNCTION_PASS("bounds-checking", BoundsCheckingPass())
 FUNCTION_PASS("break-crit-edges", BreakCriticalEdgesPass())

 FUNCTION_PASS("simplify-cfg", SimplifyCFGPass())
 FUNCTION_PASS("sink", SinkingPass())
 FUNCTION_PASS("slp-vectorizer", SLPVectorizerPass())
 FUNCTION_PASS("speculative-execution", SpeculativeExecutionPass())
 FUNCTION_PASS("spec-phis", SpeculateAroundPHIsPass())
 FUNCTION_PASS("sroa", SROA())
 FUNCTION_PASS("tailcallelim", TailCallElimPass())
 FUNCTION_PASS("unreachableblockelim", UnreachableBlockElimPass())
-FUNCTION_PASS("unroll", LoopUnrollPass())
-FUNCTION_PASS("unroll<peeling;no-runtime>",LoopUnrollPass(LoopUnrollOptions().setPeeling(true).setRuntime(false)))
+FUNCTION_PASS_PARAMETRIZED("unroll", \
+         [](LoopUnrollOptions Opts) { return LoopUnrollPass(Opts); }, \
+         LoopUnrollOptionsParser())
 FUNCTION_PASS("verify", VerifierPass())
 FUNCTION_PASS("verify<domtree>", DominatorTreeVerifierPass())
 FUNCTION_PASS("verify<loops>", LoopVerifierPass())
 FUNCTION_PASS("verify<memoryssa>", MemorySSAVerifierPass())
 FUNCTION_PASS("verify<regions>", RegionInfoVerifierPass())
 FUNCTION_PASS("view-cfg", CFGViewerPass())
 FUNCTION_PASS("view-cfg-only", CFGOnlyViewerPass())
 FUNCTION_PASS("transform-warning", WarnMissedTransformationsPass())
 #undef FUNCTION_PASS
+#undef FUNCTION_PASS_PARAMETRIZED
 
 #ifndef LOOP_ANALYSIS
 #define LOOP_ANALYSIS(NAME, CREATE_PASS)
 #endif
 LOOP_ANALYSIS("no-op-loop", NoOpLoopAnalysis())
 LOOP_ANALYSIS("access-info", LoopAccessAnalysis())
 LOOP_ANALYSIS("ivusers", IVUsersAnalysis())
 LOOP_ANALYSIS("pass-instrumentation", PassInstrumentationAnalysis(PIC))
 #undef LOOP_ANALYSIS
 
 #ifndef LOOP_PASS
 #define LOOP_PASS(NAME, CREATE_PASS)
 #endif