[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:42-45
+  LoopUnrollOptions(Optional<bool> Partial = None,
+                    Optional<bool> Peeling = None,
+                    Optional<bool> Runtime = None,
+                    Optional<bool> UpperBound = None)
----------------
Any reason to allow this? Seems to just make it harder to correctly use...


================
Comment at: include/llvm/Transforms/Scalar/LoopUnrollPass.h:49
+
+  // Use 'builder' pattern to set members by name at construction time
+
----------------
I'd fold this comment into a more general description of how to use this class in the class doxygen comment.


================
Comment at: include/llvm/Transforms/Scalar/LoopUnrollPass.h:51
+
+  /// Selector for the optional behavior - Partial unrolling.
+  LoopUnrollOptions &setPartial(bool Partial) {
----------------
Instead give a brief description of what behavior of the unroll pass it enables?

(Same for below methods...)


================
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) {}
----------------
Shouldn't the level move into the options struct as well?


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


================
Comment at: test/Transforms/LoopUnroll/runtime-loop.ll:19
 
+; COMMON-LABEL: @test(
+
----------------
Make an initial update to factor in the common stuff here, using the existing flags? Then this patch will look a bit more focused.


Repository:
  rL LLVM

https://reviews.llvm.org/D53440