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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
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 =
----------------
vdmitrie wrote:
> 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).
In units


================
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
----------------
vdmitrie wrote:
> 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}}}
Yes, it was a typo.


================
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.
----------------
vdmitrie wrote:
> 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?
1. Here comes after  - comes after in the order, we process stores (we process them in reverse order).

2. > Besides, #4 has already been vectorized. Why we would even consider it?
   This store still exist in LLVM IR (will be removed in SLP destrutor), though marked for the deletion. #4 just have the same distance to the base address and this is a mark, that we need to start a new stores sequence, because we have a conflict of distances (it does not matter, actually, if #4 is vectorized or not).

So, for #4 we try to vectorize sequence {2,3,4,5}, for #1 - only {1,2,3} (if #3 was not vectorized) or {1,2} ( if #3 was vectorized), because most likely #5 'depends' (in some way, not directly, but from the programmers logic, this most likely may happen after loop unrolling) on the store #4. And we can exclude it from the chain for #1 to save compile time.

3. It comes before in the reverse order, we process stores.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155246