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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

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



================
Comment at: include/llvm/Passes/PassBuilder.h:77
+  /// Tuning option to set loop interleaving on/off. Its default value is that
+  /// of the flag: `-interleave-loops-npm`.
+  bool InterleaveLoops;
----------------
chandlerc wrote:
> Comment needs updating.
Comment needs updating


================
Comment at: include/llvm/Passes/PassBuilder.h:77-81
+  /// 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`.
----------------
Comment needs updating.


================
Comment at: include/llvm/Passes/PassBuilder.h:78-82
+  bool InterleaveLoops;
+
+  /// Tuning option to enable/disable loop vectorization. Its default value is
+  /// that of the flag: `-vectorize-loops-npm`.
+  bool LoopVectorize;
----------------
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.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:278
 
+namespace llvm {
+cl::opt<bool>
----------------
You should be able to just define this as `llvm::SetLoopsInterleaved`?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:280-283
+    SetLoopsInterleaved("interleave-loops-local", cl::init(true), cl::Hidden,
+                           cl::desc("Run the Loop vectorization passes"));
+cl::opt<bool>
+    RunLoopVectorization("vectorize-loops", cl::init(false), cl::Hidden,
----------------
Following whatever pattern you pick above for the `PassTuningOptions`, I'd call these `EnableFooLoops` or `EnableLoopFoo` for both interleaving and vectorization.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:281
+    SetLoopsInterleaved("interleave-loops-local", cl::init(true), cl::Hidden,
+                           cl::desc("Run the Loop vectorization passes"));
+cl::opt<bool>
----------------
Update description


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:288-293
+// Current defaults:
+  : InterleaveOnlyWhenForced(false),
+    VectorizeOnlyWhenForced(false) {}
+// Expected usage of defaults:
+//: InterleaveOnlyWhenForced(!SetLoopsInterleaved),
+//  VectorizeOnlyWhenForced(!RunLoopVectorization) {}
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59723