[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: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:
> ABataev wrote:
> > 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.
> Here you operate (and refer) with indices into Stores array ref and it was not quite obvious that you rely on the fact that stores order in the array ref actually directly related to the order they come in input IR. Thanks for explanation.
> 
> I actually was not worried about accessing a deleted instruction (I know that SLP delays their actual removal).
> The question was related to this part of the code:
>         if (Pair.first <= It->first ||
>             VectorizedStores.contains(Stores[Pair.first]))
>           break;
> 
> 
Yep, this code check if the store was already vectorized or comes before the store with the same distance (#4 in the example)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155246