[PATCH] D26962: [LoadStoreVectorizer] Split the chain if the prefix is empty

[Justin Lebar via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:741
+    // No vectorization possible if the number of instructions
+    // is less than or equal to VF.
+    if (ChainSize <= VF) {
----------------
"less than or equal to VF" is redundant with `<= VF` in the code.   Can we say "is too small" or "is smaller than the vector width"?


================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:749
+    ArrayRef<Instruction *> Left = Chain.slice(0, VF);
+    ArrayRef<Instruction *> Right = Chain.slice(VF);
+    return vectorizeStoreChain(Left, InstructionsProcessed) |
----------------
Don't we know a priori that Left will not vectorize, because its vectorizable prefix equals Chain's vectorizable prefix?

Which leads to another question: Why do we chop off VF elements rather than just one?


================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:750
+    ArrayRef<Instruction *> Right = Chain.slice(VF);
+    return vectorizeStoreChain(Left, InstructionsProcessed) |
+           vectorizeStoreChain(Right, InstructionsProcessed);
----------------
These return bools, so can we use `||`?


================
Comment at: test/Transforms/LoadStoreVectorizer/AMDGPU/empty-prefix.ll:4
+; Split the chain and try again if the vectorizable
+; prefix is empty. This will vectorize 48 more scalars.
+
----------------
Can we write this comment in the form of

1) "Check that we X"
2) Something that does not rely on implementation details of the LSV?  That is, can we avoid the term "vectorizable prefix", or, if we must use it, can we define it here?


https://reviews.llvm.org/D26962