[PATCH] D53440: [LoopUnroll] allow customization for new-pass-manager version of LoopUnroll

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: include/llvm/Transforms/Scalar/LoopUnrollPass.h:85
   /// different unrolling stategies but supports all of them.
-  explicit LoopUnrollPass(int OptLevel = 2) : OptLevel(OptLevel) {}
+  explicit LoopUnrollPass(int OptLevel = 2, LoopUnrollOptions UnrollOpts = {})
+      : OptLevel(OptLevel), UnrollOpts(UnrollOpts) {}
----------------
fedor.sergeev wrote:
> chandlerc wrote:
> > Shouldn't the level move into the options struct as well?
> Well... current solution makes this interface change "backward compatible", not breaking existing invocations.
> Besides, OptLevel is rather different to all those UnrollOpts in that it does not describe a special transformation but is a rather nondescriptive tuning mechanism. To me that makes separating it quite logical.
> 
I'm not worried about API stability, especially here on a relatively new API. I'd prefer to get the API *right*.

I agree that the level based parameter isn't great, but I think it servers a purpose.

My suggestion would be to provide a factory function for the options struct which provides "reasonable defaults" of the options corresponding to optimization levels. It can make it clear that for full control, you should use the mutations, but that these give you starting positions which should be similar to what compilers that use the optimization levels provide.


================
Comment at: include/llvm/Transforms/Scalar/LoopUnrollPass.h:51
+
+  /// Selector for the optional behavior - Partial unrolling.
+  /// When partial unrolling is disabled only full unrolling is left allowed.
----------------
I think more simple prose wolud be better here and below. For example:

"Enables or disables partial unrolling. When disabled, only full unrolling is allowed."

The "Selector for ... - ...." I think doesn't read easily.


================
Comment at: lib/Passes/PassRegistry.def:220
 FUNCTION_PASS("unroll", LoopUnrollPass())
+FUNCTION_PASS("unroll-peeling",LoopUnrollPass(2, LoopUnrollOptions().setPartial(false).setPeeling(true).setRuntime(false).setUpperBound(false)))
 FUNCTION_PASS("verify", VerifierPass())
----------------
fedor.sergeev wrote:
> chandlerc wrote:
> > Rather than this, you can just make this pass support parsing parameters after the pass name? That would allow supporting more options.
> > 
> > Also will make it a bit cleaner as you can clearly start from a default state and adjust.
> I'm not sure I follow.
> Do you mean something like:
>   LoopUnrollPass(2).setPeeling(true) ?
No, I'm suggesting teaching the pass pipeline parsing to actually parse parameters here:

`unroll<O2>`, `unroll<partial,peeling,runtime>`, `unroll<O3,no-runtime>`.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:1345-1346
+    // flavors of unrolling during construction time (by setting Allow* values).
+    // As soon as the caller does not care we pass None for the corresponding
+    // optional to avoid providing an explicit choice.
+    LoopUnrollResult Result = tryToUnrollLoop(
----------------
This last bit (or maybe the entire comment) seems a bit stale with the move to an options struct.


Repository:
  rL LLVM

https://reviews.llvm.org/D53440