[PATCH] D149893: Rewrite LSV to handle longer chains.

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:62
+
+#include <algorithm>
+#include <cassert>
----------------
System includes should go last. clang-format would do this if there were no blank lines.
https://llvm.org/docs/CodingStandards.html#include-style



================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:200
+  VectorType *VecTy = dyn_cast<VectorType>(Ty);
+  return VecTy ? VecTy->getScalarType() : Ty;
+}
----------------
This method looks redundant. getScalarType already checks for vector type.



================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:236-241
+  for (auto BBI = I->getIterator(), E = I->getParent()->end(); BBI != E;
+       ++BBI) {
+    if (!InstructionsToMove.count(&*BBI))
+      continue;
+    Instruction *IM = &*BBI;
+    --BBI;
----------------
This way BBI is not decremented to be incremented again at the end of the loop.



================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:242-243
+    --BBI;
+    IM->removeFromParent();
+    IM->insertBefore(I);
+  }
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:805
+      // existing tests.  This isn't *so* bad, because at most we align to 4
+      // bytes (current value of StackAdjustedAlignment).
+      //
----------------
Does DL.getStackAlign() instead of 4 cause many negative differences in tests?



================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:873
+  // VecTy is a power of 2 and 1 byte at smallest, but VecElemTy may be smaller
+  // than 1 byte (e.g. <32 x i1>).
+  Type *VecTy = FixedVectorType::get(
----------------
VecElemTy can't be a vector type, can it?



================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:1310
+EquivalenceClassMap
+Vectorizer::collectEquivalenceClasses(BasicBlock::iterator begin,
+                                      BasicBlock::iterator end) {
----------------
Capitalize begin/end (the declaration already uses the correct style).



================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:1411
+    using IInfo = DenseMapInfo<Instruction *>;
+    static inline InstrListElem *getEmptyKey() {
+      return PtrInfo::getEmptyKey();
----------------
`inline` isn't necessary here
https://llvm.org/docs/CodingStandards.html#don-t-use-inline-when-defining-a-function-in-a-class-definition



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149893/new/

https://reviews.llvm.org/D149893