[PATCH] D19501: Add LoadStoreVectorizer pass

[Matt Arsenault via llvm-commits]

arsenm marked 2 inline comments as done.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:96
@@ +95,3 @@
+  bool isVectorizable(ArrayRef<Value *> &Chain, BasicBlock::iterator From,
+                      BasicBlock::iterator To);
+
----------------
jlebar wrote:
> This was marked as done, but it doesn't appear to have been done.  Not a big deal if that's intentional, but pointing it out in case it wasn't.
> 
> > Probably worth indicating that you expect all of the elements of Map to be loads, or all of them to be stores.
I put it on vectorizeChains. I can add it here too

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:322
@@ +321,3 @@
+    } else if (NumFound == Chain.size()) {
+      LastInstr = I.getIterator();
+      break;
----------------
asbirlea wrote:
> This should be: LastInstr = ++I; (using updated patch)
> It's always valid: either uses the next valid instruction or BB.end().
> 
> Fixes issue that isVectorizable misses testing the last instruction in the chain.
Leaving for future patch

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:376
@@ +375,3 @@
+  }
+
+  for (auto EntryMem : MemoryInstrs) {
----------------
asbirlea wrote:
> Consider asserting Chain.size() == ChainInstrs.size()?
This needs to go with the ++I patch

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:390
@@ +389,3 @@
+      // first load in the chain).
+      if (isa<StoreInst>(V) && isa<LoadInst>(VV) && VVIdx < VIdx)
+        continue;
----------------
asbirlea wrote:
> As loads are inserted at the location of the last load, either invert sign (VVIdx > VIdx) and update comment, or fix load insert location.
> The former misses vectorization opportunities, but it avoids (at least some of the) incorrect code generation.
Are you going to fix this / do you have test cases for this and below?


http://reviews.llvm.org/D19501