[PATCH] D91077: [LoopVectorizer][SVE] Vectorize a simple loop with with a scalable VF.

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/IR/IRBuilder.cpp:85
+  Module *M = GetInsertBlock()->getParent()->getParent();
+  assert (isa<ConstantInt>(Scaling) && "Expected constant integer");
+  Function *TheFn =
----------------
This could do with a quick clang-format.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2142-2149
+      auto *IntStepTy = IntegerType::get(ScalarIVTy->getContext(),
+                                         ScalarIVTy->getScalarSizeInBits());
+      Value *StartIdx =
+          createStepForVF(Builder, ConstantInt::get(IntStepTy, Part), VF);
+      if (ScalarIVTy->isFloatingPointTy())
+        StartIdx = Builder.CreateSIToFP(StartIdx, ScalarIVTy);
+      StartIdx = addFastMathFlag(Builder.CreateBinOp(
----------------
Can you add a comment saying this has to potentially construct a scalable step. And that otherwise it should be folded to a constant?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5417
 LoopVectorizationCostModel::selectVectorizationFactor(ElementCount MaxVF) {
+  // This can be fixed for scalable vectors later, because at this stage the
+  // LoopVectorizer will only consider vectorizing a loop with scalable vectors
----------------
Perhaps add FIXME:


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5626
+  //
+  // For scalable vectors we can't know if interleaving is beneficial. It may
+  // not be beneficial for small loops if none of the lanes in the second vector
----------------
Is it worth just not automatically interleaving for the moment? It might simplify things until the cost model is doing better, at least.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5667
                     << "LV: IC is " << IC << '\n'
                     << "LV: VF is " << VF.getKnownMinValue() << '\n');
   const bool AggressivelyInterleaveReductions =
----------------
Is there a better way to print this?


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

https://reviews.llvm.org/D91077