[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,
----------------
vdmitrie wrote:
> ABataev wrote:
> > 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.
> I do not have strong objections wrt to keeping it in the tree although Alexey's arguments sound very reasonable.  If debug printing is the issue then may be it worth trying to tweak debug printing routine(s) instead? dumpVectorizableTree() for example could collect this data and print it alongside with each tree node.
Better to teach reordering functions about dumping, rather than put some service data to the tree structure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125111