[PATCH] D125111: [SLP] Make reordering aware of external vectorizable scalar stores.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3528
     if (Optional<OrdersType> CurrentOrder =
             getReorderingData(*TE, /*TopToBottom=*/true)) {
       // Do not include ordering for nodes used in the alt opcode vectorization,
----------------
vporpo wrote:
> ABataev wrote:
> > vporpo wrote:
> > > ABataev wrote:
> > > > Maybe just add another analysis somewhere here instead of adding new fields etc.?
> > > We could do the check here, but I think it is a bit more explicit if we have a field in TreeEntry. Also it is very similar in nature to the other reordering data, so I think they should be represented in a similar way. It also helps with debugging because you can actually see it with a `dumpVectorizableTree()` dump just like the other reordering data. Wdyt?
> > I rather doubt that it is wise decision to waste some extra memory just to handle this corner case.
> I don't think that memory is a concern here since the VectorizableTree does not grow too large and we clear it before we build the next one. I think it is not worth making it less explicit just to save some memory. Reordering is already quite complex and without having this explicitly shown in the dump it would just make debugging harder.
> @vdmitrie any thoughts on this? 
> 
The reordering data is not stored in the tree, except for some cases, where this data is also required for correct codegen/cost estimation. I do not like the idea to keep this data in the tree without actually being used for cost/codegen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125111