[NewPassManager] Adding pass tuning options: loop vectorize.

[asbirlea]

Summary

Trying to add the plumbing necessary to add tuning options to the new pass manager.
Testing with the flags for loop vectorize.

[asbirlea]

ping.

[chandlerc]

include/llvm/Passes/PassBuilder.h

69

Maybe: "Tunable parameters for passes in the default pipelines."

70

Especially in the type name I'd like to emphasize that this isn't really for fundamental options, but for tuning.

Based on my comment below about how to use this, I would suggest: PipelineTuningOptions.

72

Make this a doxygen comment and add more details? Assume the reader has just started trying to use this from a library like Halide. What do they need to know?

74–75

For consistency and readability, let's use positive bools and just change the defaults?

74–75

Vertical whitespace and doxygen comments on each option since these are expected to be directly controlled by users?

93

Maybe PTO?

210

Given that the "none" behavior is to just use the default constructor, I'd rather just accept by value and have the default argument be the default constructed value. That will in turn let us trivially initialize things.

lib/Passes/PassBuilder.cpp

219

Why not support a CLI here?

862

If I understand correctly, the first parameter isn't about *unrolling* but about interleaving. We should be careful to use that terminology as disabling unrolling is a potentially reasonable thing to add.

lib/Passes/PassRegistry.def

201 ↗

(On Diff #191964)

I wouldn't use the tuning options here. If we want to support explicitly controlled behavior of the vectorization pass here, we should do so by *parsing* values out of the textual pipeline description much as we do for a few other passes. The tuning struct seems more appropriate for the pipeline above exclusively (and I would name it accordingly).

[hsaito]

lib/Passes/PassBuilder.cpp

862

@chandlerc, you are right about the terminology mess. It is the vectorizer that is "incorrectly" piggybacking on the unroll flag. If we are to start fixing the terminology here, we'd need to create DisableInterleaveLoops. If we are going to that direction, I suggest starting from setting the same value as DisableUnrollLoops so that fixing the terminology mess is NFC.

Potentially bigger mess is programmer education, to teach that interleaving and unrolling are different, as they often tend to result in identical ASM after instruction reordering --- but I fully agree that we still need to educate them (including compiler writers) to use the appropriate terminology.

[asbirlea]

Address comments and asking for clarifications.

[asbirlea]

lib/Passes/PassBuilder.cpp

862

Yes, that's true. I used the same naming and values as the ones in the old pass manager (including not having a cl option).

Updated now (also means the old and new pass managers are now inconsistent as far as namings, defaults etc, but it's probably preferable to update the old one to be consistent with this?).

lib/Passes/PassRegistry.def

201 ↗

(On Diff #191964)

I removed the options for now, but I'm still confused how things work TBH.

What if the defaults in the cl::opts are different from the constructor defaults?

Regarding parsing, I tried to understand how this is done for LoopUnroll and I only got more confused.
There's an -unroll-runtime "global" cl::opt in LoopUnrollPass.cpp that feeds into a TargetTransformInfo::UnrollingPreferences. In the new pass manager if you add in the pipeline -passes='unroll<runtime>', that's parsed into an LoopUnrollOptions and passed to the pass in PassRegistry.def.
It looks like the two managers intersect in tryToUnrollLoop, where (I assume based on the tests that) the explicit values that exist only for the NPM can overwrite the "global" ones. So there's two ways of setting defaults, and one can overwrite the other?

The tuning I had in mind to add next was -max-acc-licm-promotion in LICM. Ideally, I'd like that the value of this flag be used always as the default. When this is changed with the flag, use that new default. When it's changed in the PassBuilder, again, use that new default. Whereas if I add the parsing approach here, there's a "second source of a default".

I'd also prefer to not have the defaults embedded in the constructor, because, again, that's a "second source of defaults". If we later decide to change the default in the cl::opt option, then we also need to change the default in the constructor, no? (I am thinking of unsigned values here, caps, not bools that enable/disable things that are less likely to change often)

I still haven't understood how everything here works, so any kind of clarifications are very much welcome!

[asbirlea]

Attempting to merge defaults to a single cl::opt.
Please note the comments on defaults in lib/Transforms/Vectorize/LoopVectorize.cpp. Feedback there very welcome.

Details:
The default for LoopVectorize in the PassManagerBuilder is false. (opt checks for this, and sets LoopVectorize to true based on Olevel/size)
The default when creating the LoopVectorize with no arguments, is true (VectorizeOnlyWhenForced = false).

[chandlerc]

I think the approach here looks really good. Comments below.

include/llvm/Passes/PassBuilder.h

77

Comment needs updating

77–81

Comment needs updating.

78–82

I think we should name these consistently. Two options that seem equally good to me:

InterleaveLoops and VectorizeLoops

or

LoopInterleaving and LoopVectorization

I think I very mildly lean toward the second, but it is a very mild preference.

lib/Transforms/Vectorize/LoopVectorize.cpp

278 ↗

(On Diff #194188)

You should be able to just define this as llvm::SetLoopsInterleaved?

280–283 ↗

(On Diff #194188)

Following whatever pattern you pick above for the PassTuningOptions, I'd call these EnableFooLoops or EnableLoopFoo for both interleaving and vectorization.

281 ↗

(On Diff #194188)

Update description

288–293 ↗

(On Diff #194188)

Not really sure what this commenting structure is trying to convey, but I don't think you want to use commented out code. I'd just write prose in a comment on the method that describes what's going on here.

[chandlerc]

lib/Passes/PassBuilder.cpp

862

I think Alina's patch does a reasonable job of starting to fix this.

I don't think the education problem is that bad. Unrolling and interleaving produce pretty different results in LLVM.

[asbirlea]

lib/Transforms/Vectorize/LoopVectorize.cpp

288–293 ↗

(On Diff #194188)

I *really* don't want to leave this as is. It was meant as a discussion point. Sorry for not being clearer.

I wanted to use the "expected defaults", where interleaving and vectorization are enabled by default, but vectorization is disabled by default in the old pass manager.
Why is this?
[The interleaving is set to false already in the old pass manager, the DisableUnrollLoops variable.]

I guess I am asking why are having vectorization disabled by default in the old pass manager, but enabled by default when creating the LoopVectorizePass with no params?

We also have a strange (to me) behavior in opt.cpp:351:

• if an opt-specific option to disable-loop-vectorize is set, then it's disabled.
• Otherwise, if it's explicitly enabled with -vectorize-loops or -loop-vectorize (there's two?), then it's enabled.
• Otherwise, it assumes it's disabled (that's the default here...), and enables it if OptLevel > 1 && SizeLevel < 2.

So I can't really change the default to enable vectorization, without some changes here.

[asbirlea]

Address comments.

lib/Transforms/Vectorize/LoopVectorize.cpp

288–293 ↗

(On Diff #194188)

To make progress here, I moved the explanation to a comment block.
This way, this patch can go in as is since it doesn't change any defaults, it's just flag refactoring and plumbing. Any change that *does* change the defaults can be a follow up to this patch.

[asbirlea]

Rebase after change that parses vectorize flags.

[Meinersbur]

lib/Transforms/Vectorize/LoopVectorize.cpp

288–293 ↗

(On Diff #194188)

I guess I am asking why are having vectorization disabled by default in the old pass manager, but enabled by default when creating the LoopVectorizePass with no params?

Quite confusingly, clang enables loop vectorization in the legacy pipeline builder by default again. That is, opt -O3 disables LoopVectorize, but clang -O3 has it enabled.

[chandlerc]

LGTM with comments below addressed. All pretty minor.

include/llvm/Transforms/Vectorize/LoopVectorize.h

78

I'd name the variables exactly the same as the members. It'll also help people use descriptive comments when calling it.

78–86

I'd move this to be a doxygen comment for the method -- people are likely to want to understand this when using the default constructor.

lib/Transforms/Vectorize/LoopVectorize.cpp

287–288 ↗

(On Diff #195777)

stray commented out bit left over here...

288–293 ↗

(On Diff #194188)

FWIW, all of this makes "sense" now to me.

I'm pretty happy w/ you changing the defaults to be more consistent Alina, but like your approach of making that a separate patch.

And yeah, the fact that clang and opt have different defaults I think is just because we didn't wire things together in a way that would make them line up easily the first time around.

[asbirlea]

Address comments. Thank you for the review!

include/llvm/Passes/PassBuilder.h

struct PGOOptions {
   std::string ProfileFile;
   std::string CSProfileGenFile;
   std::string ProfileRemappingFile;
   PGOAction Action;
   CSPGOAction CSAction;
   bool SamplePGOSupport;
 };
 
+/// Tunable parameters for passes in the default pipelines.
+class PipelineTuningOptions {
+public:
+  /// Constructor sets pipeline tuning defaults based on cl::opts. Each option
+  /// can be set in the PassBuilder when using a LLVM as a library.
+  PipelineTuningOptions();
+
+  /// Tuning option to set loop interleaving on/off. Its default value is that
+  /// of the flag: `-interleave-loops-npm`.
+  bool InterleaveLoops;
+
+  /// Tuning option to enable/disable loop vectorization. Its default value is
+  /// that of the flag: `-vectorize-loops-npm`.
+  bool LoopVectorize;
+};
+
 /// This class provides access to building LLVM's passes.
 ///
 /// Its members provide the baseline state available to passes during their
 /// construction. The \c PassRegistry.def file specifies how to construct all
 /// of the built-in passes, and those may reference these members during
 /// construction.
 class PassBuilder {
   TargetMachine *TM;
+  PipelineTuningOptions PTO;
   Optional<PGOOptions> PGOOpt;
   PassInstrumentationCallbacks *PIC;
 
 public:
   /// A struct to capture parsed pass pipeline names.
   ///
   /// A pipeline is defined as a series of names, each of which may in itself
   /// recursively contain a nested pipeline. A name is either the name of a pass
enum OptimizationLevel {
     /// This is useful primarily when there are absolute size limitations and
     /// any effort taken to reduce the size is worth it regardless of the
     /// execution time impact. You should expect this level to produce rather
     /// slow, but very small, code.
     Oz
   };
 
   explicit PassBuilder(TargetMachine *TM = nullptr,
+                       PipelineTuningOptions PTO = PipelineTuningOptions(),
                        Optional<PGOOptions> PGOOpt = None,
                        PassInstrumentationCallbacks *PIC = nullptr)
-      : TM(TM), PGOOpt(PGOOpt), PIC(PIC) {}
+      : TM(TM), PTO(PTO), PGOOpt(PGOOpt), PIC(PIC) {}
 
   /// Cross register the analysis managers through their proxies.
   ///
   /// This is an interface that can be used to cross register each
   // AnalysisManager with all the others analysis managers.
   void crossRegisterProxies(LoopAnalysisManager &LAM,
                             FunctionAnalysisManager &FAM,
                             CGSCCAnalysisManager &CGAM,

include/llvm/Transforms/Vectorize/LoopVectorize.h

 class Loop;
 class LoopAccessInfo;
 class LoopInfo;
 class OptimizationRemarkEmitter;
 class ScalarEvolution;
 class TargetLibraryInfo;
 class TargetTransformInfo;
 
 /// The LoopVectorize Pass.
 struct LoopVectorizePass : public PassInfoMixin<LoopVectorizePass> {
+  LoopVectorizePass(bool IOWF = false, bool VOWF = false)
+      : InterleaveOnlyWhenForced(IOWF), VectorizeOnlyWhenForced(VOWF) {}
   /// If false, consider all loops for interleaving.
   /// If true, only loops that explicitly request interleaving are considered.
   bool InterleaveOnlyWhenForced = false;
 
   /// If false, consider all loops for vectorization.
   /// If true, only loops that explicitly request vectorization are considered.
   bool VectorizeOnlyWhenForced = false;
 
   ScalarEvolution *SE;
   LoopInfo *LI;
   TargetTransformInfo *TTI;
   DominatorTree *DT;
   BlockFrequencyInfo *BFI;

lib/LTO/LTOBackend.cpp

static void runNewPMPasses(Config &Conf, Module &Mod, TargetMachine *TM,
   else if (Conf.RunCSIRInstr) {
     PGOOpt = PGOOptions("", Conf.CSIRProfile, Conf.ProfileRemapping,
                         PGOOptions::IRUse, PGOOptions::CSIRInstr);
   } else if (!Conf.CSIRProfile.empty()) {
     PGOOpt = PGOOptions(Conf.CSIRProfile, "", Conf.ProfileRemapping,
                         PGOOptions::IRUse, PGOOptions::CSIRUse);
   }
 
-  PassBuilder PB(TM, PGOOpt);
+  PassBuilder PB(TM, PipelineTuningOptions(), PGOOpt);
   AAManager AA;
 
   // Parse a custom AA pipeline if asked to.
   if (auto Err = PB.parseAAPipeline(AA, "default"))
     report_fatal_error("Error parsing default AA pipeline");
 
   LoopAnalysisManager LAM(Conf.DebugPassManager);
   FunctionAnalysisManager FAM(Conf.DebugPassManager);

lib/Passes/PassBuilder.cpp

 
 static Regex DefaultAliasRegex(
     "^(default|thinlto-pre-link|thinlto|lto-pre-link|lto)<(O[0123sz])>$");
 
 static cl::opt<bool>
     EnableCHR("enable-chr-npm", cl::init(true), cl::Hidden,
               cl::desc("Enable control height reduction optimization (CHR)"));
 
+static cl::opt<bool>
+    RunLoopVectorizationNPM("vectorize-loops-npm", cl::init(true), cl::Hidden,
+                            cl::desc("Run the Loop vectorization passes"));
+static cl::opt<bool>
+    SetLoopsInterleavedNPM("interleave-loops-npm", cl::init(true), cl::Hidden,
+                           cl::desc("Run the Loop vectorization passes"));
+
+PipelineTuningOptions::PipelineTuningOptions() {
+  InterleaveLoops = SetLoopsInterleavedNPM;
+  LoopVectorize = RunLoopVectorizationNPM;
+}
+
 extern cl::opt<bool> EnableHotColdSplit;
 extern cl::opt<bool> EnableOrderFileInstrumentation;
 
 extern cl::opt<bool> FlattenedProfileUsed;
 
 static bool isOptimizingForSize(PassBuilder::OptimizationLevel Level) {
   switch (Level) {
   case PassBuilder::O0:
ModulePassManager PassBuilder::buildModuleOptimizationPipeline(
 
   // Distribute loops to allow partial vectorization.  I.e. isolate dependences
   // into separate loop that would otherwise inhibit vectorization.  This is
   // currently only performed for loops marked with the metadata
   // llvm.loop.distribute=true or when -enable-loop-distribute is specified.
   OptimizePM.addPass(LoopDistributePass());
 
   // Now run the core loop vectorizer.
-  OptimizePM.addPass(LoopVectorizePass());
+  OptimizePM.addPass(
+      LoopVectorizePass(!PTO.InterleaveLoops, !PTO.LoopVectorize));
 
   // Eliminate loads by forwarding stores from the previous iteration to loads
   // of the current iteration.
   OptimizePM.addPass(LoopLoadEliminationPass());
 
   // Cleanup after the loop optimization passes.
   OptimizePM.addPass(InstCombinePass());
 

tools/opt/NewPMDriver.cpp

if (CSPGOKindFlag != NoCSPGO) {
           errs() << "CSInstrUse needs to be together with InstrUse";
         P->CSAction = PGOOptions::CSIRUse;
       }
     }
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI;
   SI.registerCallbacks(PIC);
 
-  PassBuilder PB(TM, P, &PIC);
+  PassBuilder PB(TM, PipelineTuningOptions(), P, &PIC);
   registerEPCallbacks(PB, VerifyEachPass, DebugPM);
 
   // Load requested pass plugins and let them register pass builder callbacks
   for (auto &PluginFN : PassPlugins) {
     auto PassPlugin = PassPlugin::Load(PluginFN);
     if (!PassPlugin) {
       errs() << "Failed to load passes from '" << PluginFN
              << "'. Request ignored.\n";

unittests/IR/PassBuilderCallbacksTest.cpp

PassBuilderCallbacksTest()
                   "  %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop ]\n"
                   "  %iv.next = add i32 %iv, 1\n"
                   "  tail call void @bar()\n"
                   "  %cmp = icmp eq i32 %iv, %n\n"
                   "  br i1 %cmp, label %exit, label %loop\n"
                   "exit:\n"
                   "  ret void\n"
                   "}\n")),
-        CallbacksHandle(), PB(nullptr, None, &CallbacksHandle.Callbacks),
+        CallbacksHandle(),
+        PB(nullptr, PipelineTuningOptions(), None, &CallbacksHandle.Callbacks),
         PM(true), LAM(true), FAM(true), CGAM(true), AM(true) {
 
     /// Register a callback for analysis registration.
     ///
     /// The callback is a function taking a reference to an AnalyisManager
     /// object. When called, the callee gets to register its own analyses with
     /// this PassBuilder instance.
     PB.registerAnalysisRegistrationCallback([this](AnalysisManagerT &AM) {