[PATCH] [LoopVectorize]Teach Loop Vectorizer about interleaved memory access

[Renato Golin]

Hi Hao,

First round of comments, I haven't had time to review the whole code, but it looks like it's going in the right direction.

cheers,
--renato


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:782
@@ +781,3 @@
+  unsigned NumIterations = ForcedFactor * ForcedUnroll;
+  // The number of vectorized & interleaved iterations should not be less than 2
+  if (NumIterations < 2)
----------------
why not?

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:790
@@ +789,3 @@
+  //   (2) Distance >= TypeByteSize * NumIterations * Stride
+  // is true and
+  //   (3) Distance <= MaxSafeDepDistBytes
----------------
This comment is confusing. You mean to say that 3 always has to be true, while 1 or 2 should be true. I'd rephrase this as:

    // Positive distance is safe when:
    //    Distance <= MaxSafeDepDistBytes
    // And either one below is true:
    //   * Distance < Stride * TypeByteSize
  ​  //   * Distance >= TypeByteSize * NumIterations * Stride

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:164
@@ +163,3 @@
+static cl::opt<bool>
+    VectorizeInterleavedAccess("vectorize-interleaved-access", cl::init(true),
+                               cl::Hidden,
----------------
If we need a flag for the moment, it's to make it false by default, so we can turn it on when we want, not the other way around. Later, if the pass has proven correct, we should make it on by default. This transformation won't change unit stride loops anyway, so I don't see why we would need to disable it even temporarily.

Another note, I don't think we need the extensive comment here, just one line line the others will be fine. Nor we should document *how* we vectorize, since if that changes, the comment will be automatically obsolete, and probably never changed.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:704
@@ +703,3 @@
+          return i;
+      return Members.size();
+    }
----------------
You're already making sure the item is there. I think you should have an llvm_unreachable() here.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1833
@@ +1832,3 @@
+// If the 1st vector has more elements, extend the 2nd vector with UNDEFs.
+static Value *ConcateTwoVectors(IRBuilder<> &Builder, Value *V1, Value *V2) {
+  VectorType *VecTy1 = dyn_cast<VectorType>(V1->getType());
----------------
s/Concate/Concatenate/

I thought we had this kind of functionality... How is Elena doing the same for her indexed loads?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1859
@@ +1858,3 @@
+// Concatenate number of vectors in the give list starting from Idx
+static Value *ConcateVectors(IRBuilder<> &Builder,
+                             SmallVector<Value *, 4> &List, unsigned Idx,
----------------
s/Concate/Concatenate/

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3751
@@ -3416,1 +3750,3 @@
 
+static bool isInBoundsGep(Value *Ptr) {
+  if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(Ptr))
----------------
Verbatim copy is not right. This is too generic to not be exposed by some GEP or Builder class.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3758
@@ +3757,3 @@
+/// \brief Check whether the access through \p Ptr has a constant stride.
+static int isStridedPtr(ScalarEvolution *SE, Value *Ptr, const Loop *Lp,
+                        const SCEV *&PtrScev) {
----------------
Non-Verbatim copy is even worse. Please make sure that you can use lib/Analysis' isStridedPtr.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4039
@@ +4038,3 @@
+      if (TryMerge) {
+        collectInterleavedAccess(AccessQueue, CurStride, IsLoad);
+        CurBase = nullptr;
----------------
Potential uninitialized use of CurStride.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4081
@@ +4080,3 @@
+    // Collect the tail accesses.
+    if (CurBase != nullptr) {
+      collectInterleavedAccess(AccessQueue, CurStride, IsLoad);
----------------
You just need

    if (CurBase) {

http://reviews.llvm.org/D9368

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/