[PATCH] D59723: [NewPassManager] Adding pass tuning options: loop vectorize.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/Passes/PassBuilder.h:69
 
+/// A struct capturing Pass tunables.
+class PassOptions {
----------------
Maybe: "Tunable parameters for passes in the default pipelines."


================
Comment at: include/llvm/Passes/PassBuilder.h:70
+/// A struct capturing Pass tunables.
+class PassOptions {
+public:
----------------
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`.


================
Comment at: include/llvm/Passes/PassBuilder.h:72
+public:
+  // Constructor sets defaults based on cl::opts.
+  PassOptions();
----------------
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?


================
Comment at: include/llvm/Passes/PassBuilder.h:74-75
+  PassOptions();
+  bool DisableUnrollLoops;
+  bool LoopVectorize;
+};
----------------
For consistency and readability, let's use positive bools and just change the defaults?


================
Comment at: include/llvm/Passes/PassBuilder.h:74-75
+  PassOptions();
+  bool DisableUnrollLoops;
+  bool LoopVectorize;
+};
----------------
chandlerc wrote:
> For consistency and readability, let's use positive bools and just change the defaults?
Vertical whitespace and doxygen comments on each option since these are expected to be directly controlled by users?


================
Comment at: include/llvm/Passes/PassBuilder.h:86
   TargetMachine *TM;
+  PassOptions PassOpt;
   Optional<PGOOptions> PGOOpt;
----------------
Maybe `PTO`?


================
Comment at: include/llvm/Passes/PassBuilder.h:203
   explicit PassBuilder(TargetMachine *TM = nullptr,
+                       Optional<PassOptions> PassOpt = None,
                        Optional<PGOOptions> PGOOpt = None,
----------------
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.


================
Comment at: lib/Passes/PassBuilder.cpp:219
+PassOptions::PassOptions() {
+  DisableUnrollLoops = false;
+  LoopVectorize = RunLoopVectorizationNPM;
----------------
Why not support a CLI here?


================
Comment at: lib/Passes/PassBuilder.cpp:859
+  OptimizePM.addPass(
+      LoopVectorizePass(PassOpt.DisableUnrollLoops, !PassOpt.LoopVectorize));
 
----------------
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.


================
Comment at: lib/Passes/PassRegistry.def:201
 FUNCTION_PASS("loop-distribute", LoopDistributePass())
-FUNCTION_PASS("loop-vectorize", LoopVectorizePass())
+FUNCTION_PASS("loop-vectorize", LoopVectorizePass(PassOpt.DisableUnrollLoops, !PassOpt.LoopVectorize))
 FUNCTION_PASS("pgo-memop-opt", PGOMemOPSizeOpt())
----------------
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).


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59723/new/

https://reviews.llvm.org/D59723