[PATCH] D21934: Address two correctness issues in LoadStoreVectorizer

[Justin Lebar via llvm-commits]

jlebar added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:366
@@ -362,3 +365,3 @@
     } else if (NumFound == Chain.size()) {
-      LastInstr = I.getIterator();
+      LastInstr = ++(I.getIterator());
       break;
----------------
Can we have a comment here?  Maybe just

    // LastInstr is not inclusive.

Or alternatively maybe it would make more sense to leave this as-is and do

    // Range is [first, last).
    return std::maked_pair(FirstInstr, ++LastInstr.getIterator());

?

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:422
@@ +421,3 @@
+  assert(Chain.size() == ChainInstrs.size() &&
+         "LSV: Error occured while checking if a chain is vectorizable");
+
----------------
Nit, no need for "LSV:" -- the assertion error will point to this line of code.  And, can we make this message a bit more specific?  Like, clearly if an assert fails, there's an error.  But what is the condition we're checking here, exactly?  Maybe "Range [From, To) must contain all instructions in Chain"?

================
Comment at: test/Transforms/LoadStoreVectorizer/AMDGPU/interleaved-mayalias-store.ll:5
@@ -4,4 +4,3 @@
 
-; This is OK to vectorize the load as long as the may alias store
-; occurs before the vector load.
+; This is NOT OK to vectorize as either load may alias either store.
 
----------------
Need a comma before "as" -- it's a conjunction separating two independent clauses.

================
Comment at: test/Transforms/LoadStoreVectorizer/x86/preserve-order32.ll:1
@@ +1,2 @@
+; RUN: opt -mtriple=x86-linux -load-store-vectorizer -S -o - %s | FileCheck %s
+
----------------
Can we have a comment somewhere indicating what we're testing?  Same for the other test.  And maybe we should get rid of whatever instructions aren't relevant to the thing we're checking from our CHECKs?


http://reviews.llvm.org/D21934