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

[Hao Liu]

Updated a new patch according to Renato's comments.

Review please.

Thanks,
-Hao


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:370
@@ -369,2 +369,3 @@
   };
 
+  struct InterleaveGroup {
----------------
rengolin wrote:
> Please add some high level description of this class
Done.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:371
@@ -370,1 +370,3 @@
 
+  struct InterleaveGroup {
+    InterleaveGroup(Instruction *Instr, int Stride, unsigned Align)
----------------
rengolin wrote:
> nitpick: I'd use class with public section rather than a struct with a private section. Just to keep the C++ness of things.
Done.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:395
@@ +394,3 @@
+    ///
+    /// \returns true on success.
+    bool insertMember(Instruction *Instr, int Index, unsigned NewAlign) {
----------------
rengolin wrote:
> a better comment would be "returns false if the instruction doesn't belong to the group"
Done.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:432
@@ +431,3 @@
+
+      llvm_unreachable("Contains no such member");
+    }
----------------
rengolin wrote:
> Add "InterleaveGroup" to "contains...". A little step for the developer, a giant leap for the debugger. :)
Done.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:446
@@ +445,3 @@
+
+    DenseMap<int, Instruction *> &getMemberMap() { return Members; }
+
----------------
rengolin wrote:
> You shouldn't expose a private member like that. Otherwise, there's no point in making it private.
> 
> In removeInterleaveGroup() you erase all components and delete the group, completely bypassing the private keyword.
> 
> Why would you need to delete the Members' list and not the rest of the InterleaveGroup's members? Why not just delete the whole object and let the destructor in InterleaveGroup free Members privately?
Done.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:580
@@ +579,3 @@
+
+  void removeInterleaveGroup(InterleaveGroup *Group);
+
----------------
rengolin wrote:
> This should move inside InterleaveGroup's destructor.
Done.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:786
@@ +785,3 @@
+  unsigned NumIter =
+      std::max(ForcedFactor * ForcedUnroll, static_cast<unsigned>(2));
+
----------------
rengolin wrote:
> you can use "2U" instead of the static_cast.
Done.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:796
@@ -787,1 +795,3 @@
+        Distance >= TypeByteSize * NumIter * Stride) ||
+      Distance > MaxSafeDepDistBytes) {
     DEBUG(dbgs() << "LAA: Failure because of Positive distance "
----------------
rengolin wrote:
> I think the:
> 
>     Distance > MaxSafeDepDistBytes
> 
> part deserves a separate debug message.
Done.

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

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

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1293
@@ +1292,3 @@
+    Instruction *A = *I;
+    if (!StridedAccesses.count(A))
+      continue;
----------------
rengolin wrote:
> 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.
Very useful suggestion. I've refactored this.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1300
@@ +1299,3 @@
+    if (isAccessInterleaved(A)) {
+      Group = InterleaveGroupMap[A];
+    } else {
----------------
rengolin wrote:
> 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.
Very useful suggestion. I've refactored this.

http://reviews.llvm.org/D9368

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