[PATCH] D145163: Add support for vectorization of interleaved memory accesses for scalable VF

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/IR/IRBuilder.cpp:1285
+                                               const Twine &Name) {
+  VectorType *VecTy = dyn_cast<VectorType>(Val->getType());
+  assert(VecTy && "Tried to deinterleave a non-vector");
----------------
cast<>


================
Comment at: llvm/lib/IR/IRBuilder.cpp:1326
+                                             const Twine &Name) {
+  assert(Vals.size() == Factor && Factor != 0 &&
+         "Tried to interleave invalid number of vectors");
----------------
Given this assert, we shouldn't need to pass Factor in here at all.  


================
Comment at: llvm/lib/IR/IRBuilder.cpp:1329
+
+  VectorType *VecTy = dyn_cast<VectorType>(Vals[0]->getType());
+
----------------
cast<>


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2713
       Group->addMetadata(NewLoad);
-      NewLoads.push_back(NewLoad);
+      NewLoads.push_back(Builder.CreateVectorDeinterleave(
+          NewLoad, InterleaveFactor, "strided.vec"));
----------------
Having this be only in the normal load path seems unlikely to be correct.  Surely we must also handle masked loads as well?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2741
+        State.set(VPDefs[I], StridedVec, Part);
       }
     }
----------------
It looks like you're changing the handling for gaps in the deinterleave.  This seems surprising and worth some discussion?


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

https://reviews.llvm.org/D145163