[PATCH] D95245: [SVE] Add support for scalable vectorization of loops with int/fast FP reductions

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added a comment.

Hey Kerry, 
Thank you for this patch. 
I found some nit and I have some suggestions about instructionCost.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4695
       // first lane. Otherwise, we generate all VF values.
       unsigned Lanes =
           Cost->isUniformAfterVectorization(P, VF) ? 1 : VF.getKnownMinValue();
----------------
So once we start to use Scalable vector and we start to use the VF.getKnownMinValue(), shouldn't;t this be multiplied by getMaxVScale()?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4742
       // Create a vector of consecutive numbers from zero to VF.
       for (unsigned i = 0; i < VF.getKnownMinValue(); ++i)
         Indices.push_back(
----------------
Same here, should we not need to multiply by getMaxVScale()?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6177
 
-  assert(LoopCost && "Non-zero loop cost expected");
+  assert(LoopCost.getValue() && "Non-zero loop cost expected");
 
----------------
I believe we can use LoopCost.isValid(), here!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6198
       TTI.enableAggressiveInterleaving(HasReductions);
-  if (!InterleavingRequiresRuntimePointerCheck && LoopCost < SmallLoopCost) {
+  if (!InterleavingRequiresRuntimePointerCheck && (unsigned)*LoopCost.getValue() < SmallLoopCost) {
     // We assume that the cost overhead is 1 and we use the cost model
----------------
Can you change SmallLoopCost to be instruction cost as LoopCost, so you don't need to use *LoopCost.getValue()?
And I believe that in the std::min you will not need to use getValue


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7677
     // loop below supports scalable VFs.
+
     ElementCount VF = UserVFIsLegal ? UserVF : MaxVF;
----------------
nit


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9441
+    if (VF.Cost.isValid())
+      IC = CM.selectInterleaveCount(VF.Width, VF.Cost);
   }
----------------
nit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95245