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

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev added inline comments.


================
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.
----------------
chandlerc wrote:
> 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.
"Simple english prose" is not my strong point ;) Thanks for suggestion.


================
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())
----------------
chandlerc wrote:
> 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>`.
Ah, okey, that should work.


Repository:
  rL LLVM

https://reviews.llvm.org/D53440