[PATCH] D155246: [SLP]Improve stores vectorization.

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12273
 
-  int E = Stores.size();
-  SmallBitVector Tails(E, false);
-  int MaxIter = MaxStoreLookup.getValue();
-  SmallVector<std::pair<int, int>, 16> ConsecutiveChain(
-      E, std::make_pair(E, INT_MAX));
-  SmallVector<SmallBitVector, 4> CheckedPairs(E, SmallBitVector(E, false));
-  int IterCnt;
-  auto &&FindConsecutiveAccess = [this, &Stores, &Tails, &IterCnt, MaxIter,
-                                  &CheckedPairs,
-                                  &ConsecutiveChain](int K, int Idx) {
-    if (IterCnt >= MaxIter)
-      return true;
-    if (CheckedPairs[Idx].test(K))
-      return ConsecutiveChain[K].second == 1 &&
-             ConsecutiveChain[K].first == Idx;
-    ++IterCnt;
-    CheckedPairs[Idx].set(K);
-    CheckedPairs[K].set(Idx);
-    std::optional<int> Diff = getPointersDiff(
-        Stores[K]->getValueOperand()->getType(), Stores[K]->getPointerOperand(),
-        Stores[Idx]->getValueOperand()->getType(),
-        Stores[Idx]->getPointerOperand(), *DL, *SE, /*StrictCheck=*/true);
-    if (!Diff || *Diff == 0)
-      return false;
-    int Val = *Diff;
-    if (Val < 0) {
-      if (ConsecutiveChain[Idx].second > -Val) {
-        Tails.set(K);
-        ConsecutiveChain[Idx] = std::make_pair(K, -Val);
+  DenseSet<std::pair<Value *, Value *>> TriedSequences;
+  auto TryToVectorize = [&](const auto &Set) {
----------------
Please add a  description of what is stored in the set.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12274
+  DenseSet<std::pair<Value *, Value *>> TriedSequences;
+  auto TryToVectorize = [&](const auto &Set) {
+    int PrevDist = -1;
----------------
Could you please specify actual type of the argument here and also add some description (and specifically what captured data it updates)? 


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12353
+      std::pair<unsigned, std::set<std::pair<unsigned, int>, StoreDistCompare>>>
+      SortedStores;
+  auto FillStoresSet = [&](unsigned Idx, StoreInst *SI) {
----------------
Need a description of what is stored in the container, a quick intro of how to interpret data.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12354
+      SortedStores;
+  auto FillStoresSet = [&](unsigned Idx, StoreInst *SI) {
+    for (auto &Set : SortedStores) {
----------------
Please add a description of the lambda?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12370
+      TryToVectorize(Set.second);
+      decltype(Set.second) PrevSet;
+      PrevSet.swap(Set.second);
----------------
I believe we can avoid abusing this c++ feature here. This is okay for template libraries for reason but here it makes very difficult to read code as it forcing to wade in code in search for the actual type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155246