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

[Michael Zolotukhin]

Hi Hao,

Please find some comments from me inline. I'll get back to review a bit later.

Also, I think it's a good idea to separate the part for LoopAccessAnalysis part into a separate patch. I think it's fine to keep it in one patch during review (to keep the history), but I'd suggest committing it separately when they are ready. It'll help tracking down any potential issues that could be exposed later.

Thanks,
Michael


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:325
@@ +324,3 @@
+      : Align(Align), SmallestKey(0), LargestKey(0), InsertPos(nullptr) {
+    assert(Align && "The alignment should be non-zero");
+
----------------
We require `Align!=0` here, but in the constructor below we explicitly set it to 0. Is one of these unused and could be removed?

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:495-496
@@ +494,4 @@
+  /// \returns the newly created interleave group.
+  InterleaveGroup *createInterleaveGroup(Instruction *Instr, int Stride,
+                                                unsigned Align) {
+    assert(!InterleaveGroupMap.count(Instr) &&
----------------
Please fix the indentation here.

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:448
@@ +447,3 @@
+  /// \return The cost of one member in the interleaved access.
+  /// \p Vecty is the wide vecotor type of the whole interleaved access.
+  /// \p SubTy is the vector type of the member.
----------------
s/vecotor/vector/

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:449-451
@@ +448,5 @@
+  /// \p Vecty is the wide vecotor type of the whole interleaved access.
+  /// \p SubTy is the vector type of the member.
+  /// \p Index is the index of current member in the interleaved access.
+  /// \p Gap is the number of missing members
+  unsigned getInterleavedMemoryOpCost(unsigned Opcode, Type *VecTy,
----------------
This comment seems to be outdated - there are no arguments `SubTy`, `Index`, or `Gap` in the declaration below.

================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:306
@@ +305,3 @@
+                                      unsigned AddressSpace) {
+    return Delta;
+  }
----------------
Why the cost is `Delta`?

================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:534
@@ +533,3 @@
+
+    // The memory op cost is equal to the cost of the whole wide vector divied
+    // by the actual number of members.
----------------
s/divied/divided/

================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:546-547
@@ +545,4 @@
+
+      for (unsigned i = 0; i < NumElts; i++)
+        Cost += getVectorInstrCost(Instruction::ExtractElement, VT, i);
+
----------------
Am I getting it correctly, that `VecTy` is the type of 'combined' vector?
I.e. if we have a stride=2, VF=4, and ScalarTy=i32, then `VecTy` would be `<8 x i32>`, and `SubVT` would be `<4 x i32>`? I'm not sure I've completely understood this part, so please correct me if I'm wrong.

If that's true, then the cost computation doesn't seem correct. Extract 1st or 2nd element from the wide vector isn't the same as extracting odds or even elements.

================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:551
@@ +550,3 @@
+      for (unsigned i = 0; i < NumSubElts; i++)
+        InsSubCost = getVectorInstrCost(Instruction::InsertElement, SubVT, i);
+
----------------
Was it supposed to be `+=`?

================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:558-564
@@ +557,9 @@
+
+      unsigned ExtSubCost = 0;
+      for (unsigned i = 0; i < NumSubElts; i++)
+        ExtSubCost = getVectorInstrCost(Instruction::ExtractElement, SubVT, i);
+      Cost += Delta * ExtSubCost;
+
+      for (unsigned i = 0; i < NumElts; i++)
+        Cost += getVectorInstrCost(Instruction::InsertElement, VT, i);
+    }
----------------
What's the difference between computing costs for stores and loads?

http://reviews.llvm.org/D9368

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