[PATCH] D30653: [LV] Refactor Cost Model's selectVectorizationFactor(), driven by a LoopVectorizationPlanner

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6163
 
-LoopVectorizationCostModel::VectorizationFactor
-LoopVectorizationCostModel::selectVectorizationFactor(bool OptForSize) {
-  // Width 1 means no vectorize
-  VectorizationFactor Factor = {1U, 0U};
-  if (OptForSize && Legal->getRuntimePointerChecking()->Need) {
+unsigned LoopVectorizationCostModel::computeMaxVFOrFail(bool OptForSize) {
+  const unsigned Fail = 0;
----------------
mkuper wrote:
> Ayal wrote:
> > mkuper wrote:
> > > Any reason not to use Optional<> instead?
> > Ahh, sure, done.
> > 
> > Any suggestion for a better method name, avoiding having two computeMaxVF()'s?
> Not really, since we're sort of mixing VF selection with legality in this function. :-\
These checks could potentially move into Legality, but in some sense they are "early pruning" due to excessive cost, rather than "legal" obstacles that cannot be handled. Plus only (Max)VF is considered here, not (Max)UF.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6201
+
+  if (TC % MaxVF != 0) {
+    // If the trip count that we found modulo the vectorization factor is not
----------------
mkuper wrote:
> I understand this is the way it has always been, and isn't changing in this patch, but now I realize it's fairly odd.
> Why are we bailing on "TC % MaxVF != 0" instead of trying to reduce MaxVF so that it actually is 0?
> 
> Am I missing something here? If not, could you add a FIXME, and/or fix it in a follow-up commit?
> 
It would indeed be better to search for a smaller MaxVF that does divide TC, instead of giving up. Added a FIXME.

We should also check if loop requiresScalarEpilog(), which in turn should be determined more accurately per-VF. Added another FIXME.


https://reviews.llvm.org/D30653