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

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added a comment.

Thanks for the update, Alexey. It was very much helpful.
I just have few questions



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12288-12289
   };
-  // Do a quadratic search on all of the given stores in reverse order and find
-  // all of the pairs of stores that follow each other.
-  for (int Idx = E - 1; Idx >= 0; --Idx) {
-    // If a store has multiple consecutive store candidates, search according
-    // to the sequence: Idx-1, Idx+1, Idx-2, Idx+2, ...
-    // This is because usually pairing with immediate succeeding or preceding
-    // candidate create the best chance to find slp vectorization opportunity.
-    const int MaxLookDepth = std::max(E - Idx, Idx + 1);
-    IterCnt = 0;
-    for (int Offset = 1, F = MaxLookDepth; Offset < F; ++Offset)
-      if ((Idx >= Offset && FindConsecutiveAccess(Idx - Offset, Idx)) ||
-          (Idx + Offset < E && FindConsecutiveAccess(Idx + Offset, Idx)))
-        break;
-  }
-
-  // Tracks if we tried to vectorize stores starting from the given tail
-  // already.
-  SmallBitVector TriedTails(E, false);
-  // For stores that start but don't end a link in the chain:
-  for (int Cnt = E; Cnt > 0; --Cnt) {
-    int I = Cnt - 1;
-    if (ConsecutiveChain[I].first == E || Tails.test(I))
-      continue;
-    // We found a store instr that starts a chain. Now follow the chain and try
-    // to vectorize it.
+  // A set of pairs (index of store in Stores array ref, Distance to the first
+  // store in the set).
+  using StoreIndexToDistSet =
----------------
How about the following?

"Distance to the first store in the set"
-->
"Distance of the store address relative to base store address" 

whether distance is  measured in units or bytes? (condition check "Data.second - PrevDist == 1"  suggests units).


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12363-12364
 
-    unsigned MaxVecRegSize = R.getMaxVecRegSize();
-    unsigned EltSize = R.getVectorElementSize(Operands[0]);
-    unsigned MaxElts = llvm::bit_floor(MaxVecRegSize / EltSize);
-
-    unsigned MaxVF = std::min(R.getMaximumVF(EltSize, Instruction::Store),
-                              MaxElts);
-    auto *Store = cast<StoreInst>(Operands[0]);
-    Type *StoreTy = Store->getValueOperand()->getType();
-    Type *ValueTy = StoreTy;
-    if (auto *Trunc = dyn_cast<TruncInst>(Store->getValueOperand()))
-      ValueTy = Trunc->getSrcTy();
-    unsigned MinVF = TTI->getStoreMinimumVF(
-        R.getMinVF(DL->getTypeSizeInBits(ValueTy)), StoreTy, ValueTy);
-
-    if (MaxVF <= MinVF) {
-      LLVM_DEBUG(dbgs() << "SLP: Vectorization infeasible as MaxVF (" << MaxVF << ") <= "
-                        << "MinVF (" << MinVF << ")\n");
-    }
-
-    // FIXME: Is division-by-2 the correct step? Should we assert that the
-    // register size is a power-of-2?
-    unsigned StartIdx = 0;
-    for (unsigned Size = MaxVF; Size >= MinVF; Size /= 2) {
-      for (unsigned Cnt = StartIdx, E = Operands.size(); Cnt + Size <= E;) {
-        ArrayRef<Value *> Slice = ArrayRef(Operands).slice(Cnt, Size);
-        if (!VectorizedStores.count(Slice.front()) &&
-            !VectorizedStores.count(Slice.back()) &&
-            vectorizeStoreChain(Slice, R, Cnt, MinVF)) {
-          // Mark the vectorized stores so that we don't vectorize them again.
-          VectorizedStores.insert(Slice.begin(), Slice.end());
-          Changed = true;
-          // If we vectorized initial block, no need to try to vectorize it
-          // again.
-          if (Cnt == StartIdx)
-            StartIdx += Size;
-          Cnt += Size;
-          continue;
-        }
-        ++Cnt;
+  // Stores pair (first index of the store in the Stores, set for store
+  // index/dist to the very first store).
+  // Need to store the index of the very first store separately, since the set
----------------
May I suggest to rewrite: 
"first index of the store in the Stores, set for store index/dist to the very first store"
-->
"first: index of the store into Stores array ref, address of which taken as base,
second: sorted set of pairs {index, dist}, which are indices of stores in the set and their
store location distances relative to the base address." ?

Since "fist index" is perhaps the index of store we started analysis with but it can actually end up not the first in the set (like in your example where it is the last one).




================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12382-12383
+  // - Scan this from the last to first store. The very first bunch of stores is
+  // {5, {4, -3}, {3, -2}, {2, -1}, {5, 0}} (the element in SortedStores
+  // vector).
+  // - The next store in the list - #1 - has the same distance from store #5 as
----------------
I'd expect the element should actually be like this (if I understand it correctly):
{5, {4, -3}, {2, -2}, {3, -1}, {5, 0}}

hence should try to vectorize "4,2,3,5"
Or sequence in the example should be:
  // 1. store x, %p
  // 2. store y, %p+2
  // 3. store z, %p+1
  // 4. store a, %p
  // 5. store b, %p+3

Could you please confirm there is a typo here or clarify otherwise?
btw, extra {}  would be bit more helpful too: {5, {{4, -3}, {2, -2}, {3, -1}, {5, 0}}}


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12392-12394
+  // Store #2 can be added -> comes after store #4 with the same distance as
+  // store #1.
+  // Store #5 cannot be added - comes before store #4.
----------------
Hm, I'm confused. "comes after"  means store location comes after  or  "index in the Stores comes after"?
>From the code it looks like the latter (as store indices are compared).
Besides, #4 has already been vectorized. Why we would even consider it?

And in either case it is not clear how #5 (%p+3) would come before #4 (%p) ?

Could you please clarify?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155246