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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Awesome, comments inline!



================
Comment at: lib/Passes/PassBuilder.cpp:1248
+    return true;
+  return Name.consume_front("<") && Name.consume_back(">");
+}
----------------
As `Name` is a by-value parameter, this should just be `starts_with` and `ends_with`?


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


================
Comment at: lib/Passes/PassBuilder.cpp:1265
+  }
+  // default parameters
+  if (Params.empty())
----------------
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.
```


================
Comment at: lib/Passes/PassRegistry.def:148-150
+#ifndef FUNCTION_PASS_PARAMETRIZED
+#define FUNCTION_PASS_PARAMETRIZED(NAME, CREATE_PASS, PARSER)
+#endif
----------------
Separate the parameterized X-macros into a separate block? Seems a bit cleaner, they won't sort nicely intermingled.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53895