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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59723