[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

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

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

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

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

47

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

49

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

51

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

(Same for below methods...)

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

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

47

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

47

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.

51

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.

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

1352–1353

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

"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

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.

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;
PreservedAnalyses run(Loop &L, LoopAnalysisManager &AM,
                         LoopStandardAnalysisResults &AR, LPMUpdater &U);
 };
 
 /// 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;
+  Optional<bool> AllowPartial;
+  Optional<bool> AllowRuntime;
+  Optional<bool> AllowPeeling;
+  Optional<bool> AllowUpperBound;
 
 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) {}
 
+  /// Selector for the optional behavior - Partial unrolling.
+  /// Call with \p Partial being true to enable, false to disable.
+  /// Without calling this a global default will be used.
+  LoopUnrollPass&& setPartial(bool Partial) && { AllowPartial = Partial; return std::move(*this); }
+  /// Selector for the optional behavior - Runtime unrolling.
+  /// Call with \p Runtime being true to enable, false to disable.
+  /// Without calling this a global default will be used.
+  LoopUnrollPass&& setRuntime(bool Runtime) && { AllowRuntime = Runtime; return std::move(*this); }
+  /// Selector for the optional behavior - Peeling.
+  /// Call with \p Peeling being true to enable, false to disable.
+  /// Without calling this a global default will be used.
+  LoopUnrollPass&& setPeeling(bool Peeling) && { AllowPeeling = Peeling; return std::move(*this); }
+  /// Selector for the optional behavior - UpperBound.
+  /// Call with \p UpperBound being true to enable, false to disable.
+  /// Without calling this a global default will be used.
+  LoopUnrollPass&& setUpperBound(bool UpperBound) && { AllowUpperBound = UpperBound; return std::move(*this); }
+
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_SCALAR_LOOPUNROLLPASS_H

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.
     if (PSI && PSI->hasHugeWorkingSetSize())
       AllowPeeling = false;
     std::string LoopName = L.getName();
+    // The API here is quite complex to call and we allow to select some
+    // 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(&L, DT, &LI, SE, TTI, AC, ORE,
                         /*PreserveLCSSA*/ true, OptLevel, /*Count*/ None,
-                        /*Threshold*/ None, AllowPartialParam, RuntimeParam,
-                        UpperBoundParam, AllowPeeling);
+                        /*Threshold*/ None, AllowPartial, AllowRuntime,
+                        AllowUpperBound, AllowPeeling);
     Changed |= Result != LoopUnrollResult::Unmodified;
 
     // The parent must not be damaged by unrolling!
 #ifndef NDEBUG
     if (Result != LoopUnrollResult::Unmodified && ParentL)
       ParentL->verifyLoop();
 #endif
 
     // Clear any cached analysis results for L if we removed it completely.
     if (LAM && Result == LoopUnrollResult::FullyUnrolled)
       LAM->clear(L, LoopName);
   }
 
   if (!Changed)
     return PreservedAnalyses::all();
 
   return getLoopPassPreservedAnalyses();
 }