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

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper 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;
----------------
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. :-\


================
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
----------------
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?



https://reviews.llvm.org/D30653