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

[fedor.sergeev]

Summary

Unlike its legacy counterpart new pass manager's LoopUnrollPass does
not provide any means to select which flavors of unroll to run
(runtime, peeling, partial), relying on global defaults.

In some cases having ability to run a restricted LoopUnroll that
does more than LoopFullUnroll is needed.

Introduced LoopUnrollOptions to select optional unroll behaviors.
Added 'unroll<peeling>' to PassRegistry mainly for the sake of testing.

[fedor.sergeev]

I know that this change lacks testing, yet I have no idea on how to organize the test.
I'm doing this change for our downstream usage, so there currently no use of customized loop-unroll upstream and there are no command-line controls that could trigger it in a test.

Any suggestions on how to test it?

[fedor.sergeev]

clang-formatted

[tejohnson]

In D53440#1269274, @fedor.sergeev wrote:

I know that this change lacks testing, yet I have no idea on how to organize the test.
I'm doing this change for our downstream usage, so there currently no use of customized loop-unroll upstream and there are no command-line controls that could trigger it in a test.

Any suggestions on how to test it?

I guess you could use a unittest that creates the pass and invokes it. But probably easier would be to create multiple instances of the LoopUnrollPass in the PassRegistry.def, that construct the pass with different options (see my comment below about passing the options via the constructor), and then invoke the different types of unrolling via "opt -passes=" in a test. See for example in PassRegistry.def "early-cse" vs "early-cse-memssa".

include/llvm/Transforms/Scalar/LoopUnrollPass.h

49 ↗

(On Diff #170214)

I think the usual mechanism for the new PM is to pass options to the pass constructor. E.g. see InlinerPass.

[fedor.sergeev]

include/llvm/Transforms/Scalar/LoopUnrollPass.h

49 ↗

(On Diff #170214)

I really dislike passing a bunch of booleans and then adding comments to name them, like all the callers of tryToUnrollLoop are doing. :(

[fedor.sergeev]

include/llvm/Transforms/Scalar/LoopUnrollPass.h

49 ↗

(On Diff #170214)

I can do something similar to SimplifyCFGOptions, passing the object that describes options to the constructor.

[fedor.sergeev]

updated as per comments, tests added

[fedor.sergeev]

one stray comment updated

[fedor.sergeev]

ping?

[chandlerc]

include/llvm/Transforms/Scalar/LoopUnrollPass.h

42–45 ↗

(On Diff #170617)

Any reason to allow this? Seems to just make it harder to correctly use...

49 ↗

(On Diff #170617)

I'd fold this comment into a more general description of how to use this class in the class doxygen comment.

51 ↗

(On Diff #170617)

Instead give a brief description of what behavior of the unroll pass it enables?

(Same for below methods...)

85 ↗

(On Diff #170617)

Shouldn't the level move into the options struct as well?

lib/Passes/PassRegistry.def

220 ↗

(On Diff #170617)

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.

test/Transforms/LoopUnroll/runtime-loop.ll

19 ↗

(On Diff #170617)

Make an initial update to factor in the common stuff here, using the existing flags? Then this patch will look a bit more focused.

[fedor.sergeev]

include/llvm/Transforms/Scalar/LoopUnrollPass.h

42–45 ↗

(On Diff #170617)

Well, different people have different opinions.
Personally I would gladly drop this constructor :)

85 ↗

(On Diff #170617)

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.

lib/Passes/PassRegistry.def

220 ↗

(On Diff #170617)

I'm not sure I follow.
Do you mean something like:

LoopUnrollPass(2).setPeeling(true) ?

[fedor.sergeev]

updating as per comments: removing constructor, adding comments, splitting off NFC test update

[chandlerc]

include/llvm/Transforms/Scalar/LoopUnrollPass.h

51 ↗

(On Diff #171500)

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.

85 ↗

(On Diff #170617)

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.

lib/Passes/PassRegistry.def

220 ↗

(On Diff #170617)

No, I'm suggesting teaching the pass pipeline parsing to actually parse parameters here:

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

lib/Transforms/Scalar/LoopUnrollPass.cpp

1345–1346 ↗

(On Diff #171500)

This last bit (or maybe the entire comment) seems a bit stale with the move to an options struct.

[fedor.sergeev]

include/llvm/Transforms/Scalar/LoopUnrollPass.h

51 ↗

(On Diff #171500)

"Simple english prose" is not my strong point ;) Thanks for suggestion.

lib/Passes/PassRegistry.def

220 ↗

(On Diff #170617)

Ah, okey, that should work.

[chandlerc]

include/llvm/Transforms/Scalar/LoopUnrollPass.h

51 ↗

(On Diff #171500)

It's all good, I also struggle but in other directions of the same space. My English prose tends to ramble on and on (much like this comment), often saying far too little and completely failing to use commas correctly (also much like this comment). ;]

Honestly, *reviewing* for this is always much easier than writing it in the first place. So please don't ever feel bad for any comments in code review. I'm just really excited by all the work here.

[fedor.sergeev]

updating as per comments, generic unroll parameters parsing part should be done as a followup soon after

[chandlerc]

I think this is fine to go in, a minor suggestion inline. Rest can be follow-up.

lib/Passes/PassRegistry.def

218 ↗

(On Diff #171705)

Any reason you can't just do the peeling bit? Seems overly verbose to reset the rest.

[fedor.sergeev]

lib/Passes/PassRegistry.def

218 ↗

(On Diff #171705)

Because it needs to be a restricted set, not allowing generic unroll.
At least it needs no-runtime. Will try to keep it to the minimal set.

llvm/trunk/include/llvm/Transforms/Scalar/LoopUnrollPass.h

 //===- LoopUnrollPass.h -----------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TRANSFORMS_SCALAR_LOOPUNROLLPASS_H
 #define LLVM_TRANSFORMS_SCALAR_LOOPUNROLLPASS_H
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/Analysis/LoopAnalysisManager.h"
 #include "llvm/IR/PassManager.h"
 
 namespace llvm {
 
 class Function;
 class Loop;
 class LPMUpdater;
 
 /// Loop unroll pass that only does full loop unrolling.
 class LoopFullUnrollPass : public PassInfoMixin<LoopFullUnrollPass> {
   const int OptLevel;
 
 public:
   explicit LoopFullUnrollPass(int OptLevel = 2) : OptLevel(OptLevel) {}
 
   PreservedAnalyses run(Loop &L, LoopAnalysisManager &AM,
                         LoopStandardAnalysisResults &AR, LPMUpdater &U);
 };
 
+/// A set of parameters used to control various transforms performed by the
+/// LoopUnroll pass. Each of the boolean parameters can be set to:
+///      true - enabling the transformation.
+///      false - disabling the transformation.
+///      None - relying on a global default.
+///
+/// There is also OptLevel parameter, which is used for additional loop unroll
+/// tuning.
+///
+/// Intended use is to create a default object, modify parameters with
+/// additional setters and then pass it to LoopUnrollPass.
+///
+struct LoopUnrollOptions {
+  Optional<bool> AllowPartial;
+  Optional<bool> AllowPeeling;
+  Optional<bool> AllowRuntime;
+  Optional<bool> AllowUpperBound;
+  int OptLevel;
+
+  LoopUnrollOptions(int OptLevel = 2) : OptLevel(OptLevel) {}
+
+  /// Enables or disables partial unrolling. When disabled only full unrolling
+  /// is allowed.
+  LoopUnrollOptions &setPartial(bool Partial) {
+    AllowPartial = Partial;
+    return *this;
+  }
+
+  /// Enables or disables unrolling of loops with runtime trip count.
+  LoopUnrollOptions &setRuntime(bool Runtime) {
+    AllowRuntime = Runtime;
+    return *this;
+  }
+
+  /// Enables or disables loop peeling.
+  LoopUnrollOptions &setPeeling(bool Peeling) {
+    AllowPeeling = Peeling;
+    return *this;
+  }
+
+  /// Enables or disables the use of trip count upper bound
+  /// in loop unrolling.
+  LoopUnrollOptions &setUpperBound(bool UpperBound) {
+    AllowUpperBound = UpperBound;
+    return *this;
+  }
+
+  // Sets "optimization level" tuning parameter for loop unrolling.
+  LoopUnrollOptions &setOptLevel(int O) {
+    OptLevel = O;
+    return *this;
+  }
+};
+
 /// Loop unroll pass that will support both full and partial unrolling.
 /// It is a function pass to have access to function and module analyses.
 /// It will also put loops into canonical form (simplified and LCSSA).
 class LoopUnrollPass : public PassInfoMixin<LoopUnrollPass> {
-  const int OptLevel;
+  LoopUnrollOptions UnrollOpts;
 
 public:
   /// This uses the target information (or flags) to control the thresholds for
   /// different unrolling stategies but supports all of them.
-  explicit LoopUnrollPass(int OptLevel = 2) : OptLevel(OptLevel) {}
+  explicit LoopUnrollPass(LoopUnrollOptions UnrollOpts = {})
+      : UnrollOpts(UnrollOpts) {}
 
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_SCALAR_LOOPUNROLLPASS_H

llvm/trunk/lib/Passes/PassBuilder.cpp

PassBuilder::buildModuleOptimizationPipeline(OptimizationLevel Level,
   // FIXME: It would be really good to use a loop-integrated instruction
   // combiner for cleanup here so that the unrolling and LICM can be pipelined
   // across the loop nests.
   // We do UnrollAndJam in a separate LPM to ensure it happens before unroll
   if (EnableUnrollAndJam) {
     OptimizePM.addPass(
         createFunctionToLoopPassAdaptor(LoopUnrollAndJamPass(Level)));
   }
-  OptimizePM.addPass(LoopUnrollPass(Level));
+  OptimizePM.addPass(LoopUnrollPass(LoopUnrollOptions(Level)));
   OptimizePM.addPass(InstCombinePass());
   OptimizePM.addPass(RequireAnalysisPass<OptimizationRemarkEmitterAnalysis, Function>());
   OptimizePM.addPass(createFunctionToLoopPassAdaptor(LICMPass(), DebugLogging));
 
   // Now that we've vectorized and unrolled loops, we may have more refined
   // alignment information, try to re-derive it here.
   OptimizePM.addPass(AlignmentFromAssumptionsPass());
 

llvm/trunk/lib/Passes/PassRegistry.def

 FUNCTION_PASS("sink", SinkingPass())
 FUNCTION_PASS("slp-vectorizer", SLPVectorizerPass())
 FUNCTION_PASS("speculative-execution", SpeculativeExecutionPass())
 FUNCTION_PASS("spec-phis", SpeculateAroundPHIsPass())
 FUNCTION_PASS("sroa", SROA())
 FUNCTION_PASS("tailcallelim", TailCallElimPass())
 FUNCTION_PASS("unreachableblockelim", UnreachableBlockElimPass())
 FUNCTION_PASS("unroll", LoopUnrollPass())
+FUNCTION_PASS("unroll<peeling;no-runtime>",LoopUnrollPass(LoopUnrollOptions().setPeeling(true).setRuntime(false)))
 FUNCTION_PASS("verify", VerifierPass())
 FUNCTION_PASS("verify<domtree>", DominatorTreeVerifierPass())
 FUNCTION_PASS("verify<loops>", LoopVerifierPass())
 FUNCTION_PASS("verify<memoryssa>", MemorySSAVerifierPass())
 FUNCTION_PASS("verify<regions>", RegionInfoVerifierPass())
 FUNCTION_PASS("view-cfg", CFGViewerPass())
 FUNCTION_PASS("view-cfg-only", CFGOnlyViewerPass())
 #undef FUNCTION_PASS

llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp

while (!Worklist.empty()) {
     // from back to front so that we work forward across the CFG, which
     // for unrolling is only needed to get optimization remarks emitted in
     // a forward order.
     Loop &L = *Worklist.pop_back_val();
 #ifndef NDEBUG
     Loop *ParentL = L.getParentLoop();
 #endif
 
-    // The API here is quite complex to call, but there are only two interesting
-    // states we support: partial and full (or "simple") unrolling. However, to
-    // enable these things we actually pass "None" in for the optional to avoid
-    // providing an explicit choice.
-    Optional<bool> AllowPartialParam, RuntimeParam, UpperBoundParam,
-        AllowPeeling;
     // Check if the profile summary indicates that the profiled application
     // has a huge working set size, in which case we disable peeling to avoid
     // bloating it further.
+    Optional<bool> LocalAllowPeeling = UnrollOpts.AllowPeeling;
     if (PSI && PSI->hasHugeWorkingSetSize())
-      AllowPeeling = false;
+      LocalAllowPeeling = false;
     std::string LoopName = L.getName();
-    LoopUnrollResult Result =
-        tryToUnrollLoop(&L, DT, &LI, SE, TTI, AC, ORE,
-                        /*PreserveLCSSA*/ true, OptLevel, /*Count*/ None,
-                        /*Threshold*/ None, AllowPartialParam, RuntimeParam,
-                        UpperBoundParam, AllowPeeling);
+    // The API here is quite complex to call and we allow to select some
+    // flavors of unrolling during construction time (by setting UnrollOpts).
+    LoopUnrollResult Result = tryToUnrollLoop(
+        &L, DT, &LI, SE, TTI, AC, ORE,
+        /*PreserveLCSSA*/ true, UnrollOpts.OptLevel, /*Count*/ None,
+        /*Threshold*/ None, UnrollOpts.AllowPartial, UnrollOpts.AllowRuntime,
+        UnrollOpts.AllowUpperBound, LocalAllowPeeling);
     Changed |= Result != LoopUnrollResult::Unmodified;
 
     // The parent must not be damaged by unrolling!
 #ifndef NDEBUG
     if (Result != LoopUnrollResult::Unmodified && ParentL)
       ParentL->verifyLoop();
 #endif
 

llvm/trunk/test/Transforms/LoopUnroll/peel-loop.ll

 ; RUN: opt < %s -S -loop-unroll -unroll-force-peel-count=3 -verify-dom-info -simplifycfg -instcombine | FileCheck %s
+; RUN: opt < %s -S -passes='require<opt-remark-emit>,unroll,simplify-cfg,instcombine' -unroll-force-peel-count=3 -verify-dom-info | FileCheck %s
+; RUN: opt < %s -S -passes='require<opt-remark-emit>,unroll<peeling;no-runtime>,simplify-cfg,instcombine' -unroll-force-peel-count=3 -verify-dom-info | FileCheck %s
 
 ; Basic loop peeling - check that we can peel-off the first 3 loop iterations
 ; when explicitly requested.
 ; CHECK-LABEL: @basic
 ; CHECK: %[[CMP0:.*]] = icmp sgt i32 %k, 0
 ; CHECK: br i1 %[[CMP0]], label %[[NEXT0:.*]], label %for.end
 ; CHECK: [[NEXT0]]:
 ; CHECK: store i32 0, i32* %p, align 4

llvm/trunk/test/Transforms/LoopUnroll/runtime-loop.ll

 ; RUN: opt < %s -S -loop-unroll -unroll-runtime=true -unroll-runtime-epilog=true  | FileCheck %s -check-prefixes=EPILOG,COMMON
 ; RUN: opt < %s -S -loop-unroll -unroll-runtime=true -unroll-runtime-epilog=false | FileCheck %s -check-prefixes=PROLOG,COMMON
-
+;
 ; RUN: opt < %s -S -passes='require<opt-remark-emit>,unroll' -unroll-runtime=true -unroll-runtime-epilog=true  | FileCheck %s -check-prefixes=EPILOG,COMMON
 ; RUN: opt < %s -S -passes='require<opt-remark-emit>,unroll' -unroll-runtime=true -unroll-runtime-epilog=false | FileCheck %s -check-prefixes=PROLOG,COMMON
+;
+; Restricted versions of unroll (unroll<peeling;noruntime>, unroll-full) should not be doing runtime unrolling
+; even if it is globally enabled through -unroll-runtime option
+;
+; RUN: opt < %s -S -passes='require<opt-remark-emit>,unroll<peeling;no-runtime>' -unroll-runtime=true -unroll-runtime-epilog=true  | FileCheck %s -check-prefixes=NOEPILOG,COMMON
+; RUN: opt < %s -S -passes='require<opt-remark-emit>,unroll<peeling;no-runtime>' -unroll-runtime=true -unroll-runtime-epilog=false | FileCheck %s -check-prefixes=NOPROLOG,COMMON
+; RUN: opt < %s -S -passes='require<opt-remark-emit>,loop(unroll-full)' -unroll-runtime=true -unroll-runtime-epilog=true  | FileCheck %s -check-prefixes=NOEPILOG,COMMON
+; RUN: opt < %s -S -passes='require<opt-remark-emit>,loop(unroll-full)' -unroll-runtime=true -unroll-runtime-epilog=false | FileCheck %s -check-prefixes=NOPROLOG,COMMON
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 
 ; Tests for unrolling loops with run-time trip counts
 
 ; COMMON-LABEL: @test(
 
 ; EPILOG: %xtraiter = and i32 %n
 ; EPILOG:  %lcmp.mod = icmp ne i32 %xtraiter, 0
 ; EPILOG:  br i1 %lcmp.mod, label %for.body.epil.preheader, label %for.end.loopexit
 
+; NOEPILOG-NOT: %xtraiter = and i32 %n
+
 ; PROLOG: %xtraiter = and i32 %n
 ; PROLOG:  %lcmp.mod = icmp ne i32 %xtraiter, 0
 ; PROLOG:  br i1 %lcmp.mod, label %for.body.prol.preheader, label %for.body.prol.loopexit
 
+; NOPROLOG-NOT: %xtraiter = and i32 %n
+
 ; EPILOG: for.body.epil:
 ; EPILOG: %indvars.iv.epil = phi i64 [ %indvars.iv.next.epil, %for.body.epil ],  [ %indvars.iv.unr, %for.body.epil.preheader ]
 ; EPILOG:  %epil.iter.sub = sub i32 %epil.iter, 1
 ; EPILOG:  %epil.iter.cmp = icmp ne i32 %epil.iter.sub, 0
 ; EPILOG:  br i1 %epil.iter.cmp, label %for.body.epil, label %for.end.loopexit.epilog-lcssa, !llvm.loop !0
 
+; NOEPILOG: for.body:
+; NOEPILOG-NOT: for.body.epil:
+
 ; PROLOG: for.body.prol:
 ; PROLOG: %indvars.iv.prol = phi i64 [ %indvars.iv.next.prol, %for.body.prol ], [ 0, %for.body.prol.preheader ]
 ; PROLOG:  %prol.iter.sub = sub i32 %prol.iter, 1
 ; PROLOG:  %prol.iter.cmp = icmp ne i32 %prol.iter.sub, 0
 ; PROLOG:  br i1 %prol.iter.cmp, label %for.body.prol, label %for.body.prol.loopexit.unr-lcssa, !llvm.loop !0
 
+; NOPROLOG: for.body:
+; NOPROLOG-NOT: for.body.prol:
+
 
 define i32 @test(i32* nocapture %a, i32 %n) nounwind uwtable readonly {
 entry:
   %cmp1 = icmp eq i32 %n, 0
   br i1 %cmp1, label %for.end, label %for.body
 
 for.body:                                         ; preds = %for.body, %entry
   %indvars.iv = phi i64 [ %indvars.iv.next, %for.body ], [ 0, %entry ]

 }
 
 ; This is test 2007-05-09-UnknownTripCount.ll which can be unrolled now
 ; if the -unroll-runtime option is turned on
 
 ; COMMON-LABEL: @foo(
 ; EPILOG: bb72.2:
 ; PROLOG: bb72.2:
+; NOEPILOG-NOT: bb72.2:
+; NOPROLOG-NOT: bb72.2:
 
 define void @foo(i32 %trips) {
 entry:
         br label %cond_true.outer
 
 cond_true.outer:
         %indvar1.ph = phi i32 [ 0, %entry ], [ %indvar.next2, %bb72 ]
         br label %bb72

 
 
 ; Test run-time unrolling for a loop that counts down by -2.
 
 ; COMMON-LABEL: @down(
 ; EPILOG: for.body.epil:
 ; EPILOG: br i1 %epil.iter.cmp, label %for.body.epil, label %for.cond.for.end_crit_edge.epilog-lcssa
 
+; NOEPILOG: for.body:
+; NOEPILOG-NOT: for.body.epil:
+
 ; PROLOG: for.body.prol:
 ; PROLOG: br i1 %prol.iter.cmp, label %for.body.prol, label %for.body.prol.loopexit
 
+; NOPROLOG: for.body:
+; NOPROLOG-NOT: for.body.prol:
+
 define zeroext i16 @down(i16* nocapture %p, i32 %len) nounwind uwtable readonly {
 entry:
   %cmp2 = icmp eq i32 %len, 0
   br i1 %cmp2, label %for.end, label %for.body
 
 for.body:                                         ; preds = %for.body, %entry
   %p.addr.05 = phi i16* [ %incdec.ptr, %for.body ], [ %p, %entry ]
   %len.addr.04 = phi i32 [ %sub, %for.body ], [ %len, %entry ]

 }
 
 ; Test run-time unrolling disable metadata.
 ; COMMON-LABEL: @test2(
 
 ; EPILOG: for.body:
 ; EPILOG-NOT: for.body.epil:
 
+; NOEPILOG: for.body:
+; NOEPILOG-NOT: for.body.epil:
+
 ; PROLOG: for.body:
 ; PROLOG-NOT: for.body.prol:
 
+; NOPROLOG: for.body:
+; NOPROLOG-NOT: for.body.prol:
+
 define zeroext i16 @test2(i16* nocapture %p, i32 %len) nounwind uwtable readonly {
 entry:
   %cmp2 = icmp eq i32 %len, 0
   br i1 %cmp2, label %for.end, label %for.body
 
 for.body:                                         ; preds = %for.body, %entry
   %p.addr.05 = phi i16* [ %incdec.ptr, %for.body ], [ %p, %entry ]
   %len.addr.04 = phi i32 [ %sub, %for.body ], [ %len, %entry ]