[PATCH] D22119: Extended LoadStoreVectorizer to vectorize subchains.

[Justin Lebar via llvm-commits]

jlebar added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:113
@@ -112,2 +112,3 @@
   /// in the chain between \p From and \p To. The elements of \p Chain should be
   /// all loads or all stores.
+  unsigned isVectorizable(ArrayRef<Value *> Chain, BasicBlock::iterator From,
----------------
Please update comment.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:114
@@ -113,3 +113,3 @@
   /// all loads or all stores.
-  bool isVectorizable(ArrayRef<Value *> Chain, BasicBlock::iterator From,
-                      BasicBlock::iterator To);
+  unsigned isVectorizable(ArrayRef<Value *> Chain, BasicBlock::iterator From,
+                          BasicBlock::iterator To);
----------------
This probably also deserves a new name.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:453
@@ -450,3 +452,3 @@
       DEBUG(dbgs() << "LSV: Found side-effecting operation: " << *I << '\n');
-      return false;
+      return 0;
     }
----------------
Suppose we have

- load
- load
- fn_with_side_effects
- load

Don't we want to (try to) vectorize the first two loads?  Or, do you want to handle that in a separate patch?

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:460
@@ -457,7 +459,3 @@
 
-  for (auto EntryMem : MemoryInstrs) {
-    Value *V = EntryMem.first;
-    unsigned VIdx = EntryMem.second;
-    for (auto EntryChain : ChainInstrs) {
-      Value *VV = EntryChain.first;
-      unsigned VVIdx = EntryChain.second;
+  Idx = 0;
+  for (auto EntryChain : ChainInstrs) {
----------------
Nit, I'm not wild about reusing "Idx" for something different.  (I mean, it's still an index, but over a completely different list.)

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:675
@@ -661,3 +674,3 @@
 
-bool Vectorizer::vectorizeStoreChain(ArrayRef<Value *> Chain) {
-  StoreInst *S0 = cast<StoreInst>(Chain[0]);
+bool Vectorizer::vectorizeStoreChain(ArrayRef<Value *> FullChain,
+                                     SmallVector<Value *, 16> *VectorizedSet) {
----------------
Calling this param "FullChain" is weird, since when we call this recursively, we pass what I can only call a partial chain.  :)

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:701
@@ -686,1 +700,3 @@
+    VectorizedSet->append(FullChain.begin(), FullChain.end());
     return false;
+  }
----------------
Is "VectorizedSet" the right name?  In this case we're saying that we didn't vectorize anything (and we're returning false), yet we're adding these instructions to the set...

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:704
@@ +703,3 @@
+
+  // Get enclosing instructions in BB
+  BasicBlock::iterator First, Last;
----------------
If this is just describing the call to getBoundaryInstrs, I don't think it needs a comment.  If it's describing something else, can we reword it?  I don't think "enclosing" is the right word.  Maybe "Get the first and last instructions in FullChain," but that's basically exactly what getBoundaryInstrs' comment says.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:707
@@ +706,3 @@
+  std::tie(First, Last) = getBoundaryInstrs(FullChain);
+  // Get the largest vectorizable prefix of the chain
+  unsigned StopChain = isVectorizable(FullChain, First, Last);
----------------
Nit, end sentences with periods.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:714
@@ +713,3 @@
+    // Failed after the first instruction, discard it and retry the smaller
+    // chain
+    else
----------------
This indentation is very confusing.  I think it would be OK to put the comment inside the "else"?  In which case I'd suggest also putting braces (although some folks around here would leave them out, which really mystifies me).

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:716
@@ +715,3 @@
+    else
+      VectorizedSet->append(FullChain.begin(), FullChain.begin() + 1);
+    return false;
----------------
VectorizedSet->push_back(FullChain.front());

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:718
@@ +717,3 @@
+    return false;
+  }
+
----------------
I think this would be clearer as two separate if statements (repeating the "return false").  I'd rather repeat one short line than add an extra level of nesting.

```
if (StopChain == 0) {
  // No vectorization possible, bail out.
  VectorizedSet->append(...);
  return false;
}
if (StopChain == 1) {
  // Failed after the first instruction.  Discard it and try the smaller chain.
  VectorizedSet->push_back(FullChain.front());
  return false;
}
```

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:761
@@ +760,3 @@
+  // The chain is discarded for future vectorization regardless of the result
+  // below
+  VectorizedSet->append(Chain.begin(), Chain.end());
----------------
"discarded for future vectorization" is awkward.  Maybe "We won't try again to vectorize the elements of chain, regardless of whether we succeed below."


http://reviews.llvm.org/D22119