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

[Renato Golin]

Hi Hao,

This is looking more like we had done previously. I have a few comments inline after my first pass. I'll continue later.

cheers,
--renato


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

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

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

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

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:446
@@ +445,3 @@
+
+    DenseMap<int, Instruction *> &getMemberMap() { return Members; }
+
----------------
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?

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

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

================
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 "
----------------
I think the:

    Distance > MaxSafeDepDistBytes

part deserves a separate debug message.

http://reviews.llvm.org/D9368

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