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

[Renato Golin]

Last comments on LoopAccessAnalysis. They're mostly related to C++isms, as the overall logic seems correct.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1218
@@ +1217,3 @@
+
+void LoopAccessInfo::removeInterleaveGroup(InterleaveGroup *Group) {
+  auto Members = Group->getMemberMap();
----------------
Move this to InterleaveGroup class.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1276
@@ +1275,3 @@
+    StridedAccesses[I] = StrideDescriptor(Stride, Scev, Size, Align);
+  }
+
----------------
An early exit here would be good.

    if (!StridedAccess.size())
      return;


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1293
@@ +1292,3 @@
+    Instruction *A = *I;
+    if (!StridedAccesses.count(A))
+      continue;
----------------
This is a bit of a waste. I know the DenseMap lookup is quick, but going through all the instructions again is (N*logM) with N being number of instructions and M being number of Memory instructions in the map.

We're not expecting the number of memory instructions to be that great, so maybe having an array/vector of them in insertion order (guaranteed by the previous loop), you can just iterate through them in reverse order.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1300
@@ +1299,3 @@
+    if (isAccessInterleaved(A)) {
+      Group = InterleaveGroupMap[A];
+    } else {
----------------
In my prototype, I created a Groups class, which would encapsulate this logic. I did this because I don't like the unwritten dependency that isAccessInterleaved(Inst) has with InterleaveGroupMap[Inst].

If you want to avoid creating that extra class, I suggest you create a method getInterleavedAccess(Inst) which looks up and return the element, if present, or nullptr, if not. This is different from DenseMap's [] operator / lookup() method, and is what you need here.

    InterleaveGroup *Group = getInterleavedAccessGroup(A);
    if (!Group) {
      Group = createInterleavedAccessGroup(A, DesA.Stride, DesA.Align));
    }

with

    InterleaveGroup *getInterleavedAccessGroup(Inst I) {
      if (InterleaveGroupMap.count(I))
        return InterleaveGroupMap[I];
      return nullptr;
    }

and

    void createInterleavedAccessGroup(Inst I, String, Align) {
      assert(InterleaveGroupMap.count(I) == 0);
      InterleaveGroupMap[I] = new InterleaveGroup(I, Stride, Align);
      return InterleaveGroupMap[I];
    }

Though, this is basically an object in itself.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1312
@@ +1311,3 @@
+
+      if (!StridedAccesses.count(B))
+        continue;
----------------
The list of access I proposed above would also get rid of these two extra checks per iteration.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1404
@@ +1403,3 @@
+    // Remove a interleaved store with gaps.
+    removeInterleaveGroup(Group);
+  }
----------------
Yet another candidate to be in a Groups class.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1418
@@ +1417,3 @@
+    auto GroupA = HasA ? InterleaveGroupMap[A] : nullptr;
+    auto GroupB = HasB ? InterleaveGroupMap[B] : nullptr;
+    Instruction *PosA = HasA ? GroupA->getInsertPos() : A;
----------------
More code that would be simplified by having a getInterleaveGroup() method, or a separate class.

http://reviews.llvm.org/D9368

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