[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:42-45
+  LoopUnrollOptions(Optional<bool> Partial = None,
+                    Optional<bool> Peeling = None,
+                    Optional<bool> Runtime = None,
+                    Optional<bool> UpperBound = None)
----------------
chandlerc wrote:
> Any reason to allow this? Seems to just make it harder to correctly use...
Well, different people have different opinions.
Personally I would gladly drop this constructor :)


================
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) {}
----------------
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.



================
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:
> 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) ?


Repository:
  rL LLVM

https://reviews.llvm.org/D53440