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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM with comments below addressed. All pretty minor.



================
Comment at: include/llvm/Transforms/Vectorize/LoopVectorize.h:92-100
+      // The current defaults when creating the pass with no arguments are:
+      // EnableLoopInterleaving = true and EnableLoopVectorization = true.
+      // This means that interleaving default is consistent with the cl::opt
+      // flag, while vectorization is not.
+      // FIXME: The default for EnableLoopVectorization in the cl::opt
+      // should be set to true, and the corresponding change to account for this
+      // be made in opt.cpp. The initializations below will become:
----------------
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.


================
Comment at: include/llvm/Transforms/Vectorize/LoopVectorize.h:102
       : InterleaveOnlyWhenForced(false), VectorizeOnlyWhenForced(false) {}
+  LoopVectorizeOptions(bool IOWF, bool VOWF)
+      : InterleaveOnlyWhenForced(IOWF), VectorizeOnlyWhenForced(VOWF) {}
----------------
I'd name the variables exactly the same as the members. It'll also help people use descriptive comments when calling it.


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


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:287-288
+
+// LoopVectorizePass::LoopVectorizePass()
+
 /// A helper function for converting Scalar types to vector types.
----------------
stray commented out bit left over here...


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59723