[PATCH] D22119: Extended LoadStoreVectorizer to vectorize subchains.

[Justin Lebar via llvm-commits]

jlebar accepted this revision.
This revision is now accepted and ready to land.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:112
@@ +111,3 @@
+  /// \p Chain[0, Index) is the largest vectorizable chain prefix. The elements
+  /// of \p Chain should be all loads or all stores.
+  unsigned getVectorizableEnd(ArrayRef<Value *> Chain, BasicBlock::iterator From,
----------------
asbirlea wrote:
> Well, it still applies. It's still checking for the existance of those instructions but using the info differently. I shortened the comment somewhat.
I like it now.  :)

Except we should be consistent *at least within a single comment* as far as whether we're using indicative ("checks") or imperative ("return").

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:623-632
@@ -618,4 +622,12 @@
 
-  for (int Head : Heads) {
-    if (Tails.count(Head))
+  for (unsigned i = 0; i < Heads.size(); i++) {
+    int Head = Heads[i];
+    if (VectorizedValues.count(Instrs[Head]))
+      continue;
+    for (unsigned j = 0; j < Tails.size(); j++)
+      if (Head == Tails[j] && !VectorizedValues.count(Instrs[Heads[j]])) {
+        Head = -1;
+        break;
+      }
+    if (Head < 0)
       continue;
----------------
asbirlea wrote:
> For Heads it's fine to use the range-based loop (updated). For Tails I need the iterator to get Heads[TailsIterator].
OK.  I would still very much prefer to use a separate flag variable rather than repurposing Head for that purpose, though.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:435
@@ -432,1 +434,3 @@
+  unsigned InstrIdx = 0;
+  for (auto I = From, E = To; I != E; ++I, ++InstrIdx) {
     if (isa<LoadInst>(I) || isa<StoreInst>(I)) {
----------------
Do we need to declare `E`?  Can we just say `I != To`?

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:465
@@ -460,1 +464,3 @@
+      if (isa<StoreInst>(MemInstrValue) && isa<LoadInst>(ChainInstrValue)
+          && ChainInstrIdx < MemInstrIdx)
         continue;
----------------
Please be sure to clang-format the lines you've touched in this patch (e.g. with `git-clang-format HEAD^`) before submitting.


http://reviews.llvm.org/D22119