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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


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


================
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())
----------------
chandlerc wrote:
> 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).
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!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59723