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

[Renato Golin]

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1877
@@ +1876,3 @@
+      Legal->getInterleavedAccess(Instr);
+  assert(IA && IA->InsertPos == Instr && "Unexpected instruction");
+  unsigned Idx = IA->getIndex(Instr);
----------------
IIRC, this function should only be called with the first load and last store. With the assert, you'll probably force that logic up the chain.

I'm just guessing, but if you returned instead of asserting, wouldn't that work in the same way, but not needing the conditionals in the caller?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1882
@@ +1881,3 @@
+  bool Reverse = IA->Reverse;
+  unsigned Align = IA->Align;
+  // Set the default alignment if there is no alignment.
----------------
Do you need all this caching & copying?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1884
@@ +1883,3 @@
+  // Set the default alignment if there is no alignment.
+  if (!Align) {
+    const DataLayout &DL = Instr->getModule()->getDataLayout();
----------------
Potential use of uninitialised Align member. This condition could easily fail for the wrong reasons.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1886
@@ +1885,3 @@
+    const DataLayout &DL = Instr->getModule()->getDataLayout();
+    Align = DL.getABITypeAlignment(VecTy);
+  }
----------------
This could be in the constructor, if no alignment is passed, no?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1901
@@ +1900,3 @@
+    Value *NewPtr = Builder.CreateExtractElement(PtrParts[Part], Zero);
+    // Adjust the new pointer to point to the access of index 0.
+    NewPtr =
----------------
Comment before code, pls.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1903
@@ +1902,3 @@
+    NewPtr =
+        Builder.CreateGEP(NewPtr, Builder.getInt32(-static_cast<int>(Idx)));
+
----------------
Why -Idx?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1917
@@ +1916,3 @@
+  }
+
+  setDebugLocFromInst(Builder, Instr);
----------------
nitpick: better to have code+space before a new "section" (loads), instead of space+unrelated-code. ie:

    setDebugLocFromInst(Builder, Instr);
    .

instead of

    .
    setDebugLocFromInst(Builder, Instr);


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1926
@@ +1925,3 @@
+      // Interleave and split the loaded wide vector into small vectors.
+      Value *Undef = UndefValue::get(WideVecTy);
+      for (unsigned i = 0; i < NumVec; i++) {
----------------
This variable is loop invariant and load/store invariant, so could be moved out of the "if(IsLoad)" and used for both load and store blocks.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1955
@@ +1954,3 @@
+    Value *IVec = Builder.CreateShuffleVector(
+        WideVec, UndefValue::get(WideVecTy), IMask, "interleaved.vec");
+
----------------
on loads, it's called "strided.vec", on stores "interleaved.vec". Any reason to call them differently?

http://reviews.llvm.org/D9368

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