[PATCH] D19501: Add LoadStoreVectorizer pass

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi,

Some random comments after a quick first round of review inline.

Thanks,
Michael


================
Comment at: include/llvm/Transforms/Vectorize.h:147
@@ +146,3 @@
+//
+Pass *createLoadStoreVectorizerPass(unsigned VecRegSize);
+
----------------
Minor: I'd rather hardcode `128` for now, just to make the pass signature similar to existing passes (no other pass accepts an argument in `create...Pass` I think). But it probably doesn't matter as you're going to fix it in follow-ups anyway.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:77-78
@@ +76,4 @@
+  /// Returns the first and the last instructions in Chain.
+  std::pair<BasicBlock::iterator, BasicBlock::iterator>
+  getBoundaryInstrs(ArrayRef<Value *> Chain);
+
----------------
I think the second line should be indented here.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:107
@@ +106,3 @@
+
+class LoadStoreVectorizer : public FunctionPass {
+public:
----------------
Bikeshed: I'd rename `Vectorizer` to `LoadStoreVectorizer` and `LoadStoreVectorizer` to `LoadStoreVectorizerPass` os something like this.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:152-154
@@ +151,5 @@
+
+  // Don't vectorize when the attribute NoImplicitFloat is used.
+  if (F.hasFnAttribute(Attribute::NoImplicitFloat))
+    return false;
+
----------------
Should we bail out on this for all potential targets? E.g. why is it a problem for, e.g., vectorizing integer stores on x86 using `movdqa` integer instructions?

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:286
@@ +285,3 @@
+
+void Vectorizer::reorder(Instruction *I) {
+  for (User *U : I->users())
----------------
Some comments would be helpful here (and for other functions).

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:416
@@ +415,3 @@
+
+  for (BasicBlock::iterator it = BB->begin(), e = BB->end(); it != e; ++it) {
+
----------------
Range loop?

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:486-487
@@ +485,4 @@
+
+    // Process the stores in chunks of 64.
+    for (unsigned CI = 0, CE = Size; CI < CE; CI += 64) {
+      unsigned Len = std::min<unsigned>(CE - CI, 64);
----------------
Might be a good idea to parametrize `64`.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:678
@@ +677,3 @@
+
+bool Vectorizer::vectorizeLoadChain(ArrayRef<Value *> Chain) {
+  LoadInst *L0 = cast<LoadInst>(Chain[0]);
----------------
Any chance it can be somehow combined with `vectorizeStoreChain`? They look so similar..

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:729-730
@@ +728,4 @@
+      if (AllocaInst *AI = dyn_cast_or_null<AllocaInst>(V)) {
+        AI->setAlignment(4);
+        Alignment = 4;
+      } else {
----------------
Is there something special about `4`? Can we add a define/const for it?


http://reviews.llvm.org/D19501