[PATCH] D19501: Add LoadStoreVectorizer pass

[Justin Lebar via llvm-commits]

jlebar added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:499
@@ +498,3 @@
+  DEBUG(dbgs() << "LSV: Vectorizing " << Instrs.size()
+        << " instructions.\n");
+  SmallSetVector<unsigned, 16> Heads, Tails;
----------------
arsenm wrote:
> jlebar wrote:
> > Nit, It might be helpful to print out at least the first instruction in the chain.  At least, that was helpful to me when I was debugging it.
> I'm not sure why this one is even needed. The full chain is already printed out right above this by the "LSV: Loads to vectorize" DEBUG
Well, they are sort of different...that one may never appear if we find no chains or whatever.  But I take your point that this one may not be so useful.  FWIW some additional well-placed log messages might be helpful; I was debugging with a coworker a few days ago why some instructions weren't being vectorized, and it required gdb.  But we can also easily go back and add these later.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:508
@@ +507,3 @@
+  for (int i = 0, e = Instrs.size(); i < e; ++i) {
+    ConsecutiveChain[i] = ~0U;
+    for (int j = e - 1; j >= 0; --j) {
----------------
arsenm wrote:
> jlebar wrote:
> > Nit, we know that Instrs is <= 64 elements, so could we just make ConsecutiveChain an array of signed ints?  Then we could use -1 instead of ~0U (which implied to me that we were going to do some bit twiddling below).
> > 
> > Also I think you could initialize the array with
> > 
> >   int ConsecutiveChain[64] = {-1};
> > 
> > which would be clearer than setting each element in the loop, imo.
> Can't do that. it needs to be reset after the j loop
Ah, I see.  I clearly have no idea what this loop does.  :)

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:753
@@ +752,3 @@
+  // Set insert point.
+  Builder.SetInsertPoint(&*Last);
+
----------------
arsenm wrote:
> jlebar wrote:
> > A correctness-critical assumption above is that
> > 
> > > the loads are inserted at the location of the first load
> > 
> > but here it looks like the opposite?
> I had another test which I apparently forgot to include which shows the alias insertion order behavior. I've also added another which shows the insertion point.
> 
> With the insertion point as Last, the load store load store case is correct and vectorizes as
> store
> load v2
> store.
> 
> If I change the land insertion point to be before the first, it regresses and produces the incorrect
> load v2
> store v2
> 
> Maybe the comment is backwards?
> 
> Maybe the comment is backwards?

I can't manage to convince myself that those checks are correct if we do anything but what the comments say.

In the test, I see that you have

  %ld.c = load double, double addrspace(1)* %c, align 8 ; may alias store to %a
  store double 0.0, double addrspace(1)* %a, align 8
  %ld.c.idx.1 = load double, double addrspace(1)* %c.idx.1, align 8 ; may alias store to %a
  store double 0.0, double addrspace(1)* %a.idx.1, align 8

If the comments are correct and both loads may alias the first store, isn't transforming this into "store; load v2; store" (what the test is checking for) unsafe?  I grant that vectorizing both the loads and the stores is also unsafe.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:88
@@ +87,3 @@
+  /// ElementSizeBits elements in \p Chain. Break into two pieces such that the
+  /// total size of each vector half is 1, 2 or a multiple of 4 bytes.
+  /// \p Chain is expected to have more than 4 elements.
----------------
Nit, "Legalizes" and "Breaks" would be consistent with the rest of the file.  Also suggest s/vector half/piece/ -- "vector half" is kind of hard to parse.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:96
@@ +95,3 @@
+  bool isVectorizable(ArrayRef<Value *> &Chain, BasicBlock::iterator From,
+                      BasicBlock::iterator To);
+
----------------
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.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:190
@@ +189,3 @@
+
+unsigned Vectorizer::getAddressSpaceOperand(Value *I) {
+  if (LoadInst *L = dyn_cast<LoadInst>(I))
----------------
Nit, would getOperandAddressSpace() or getPointerAddressSpace() be a better name?  The address space itself isn't an operand of the instruction.


http://reviews.llvm.org/D19501