[PATCH] D90445: [SLP] Make SLPVectorizer to use `llvm.masked.gather` intrinsic

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev marked an inline comment as done.
anton-afanasyev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2847
+                                     ReuseShuffleIndicies);
+        TE->State = TreeEntry::ScatterVectorize;
+        TE->setOperandsInOrder();
----------------
ABataev wrote:
> anton-afanasyev wrote:
> > ABataev wrote:
> > > anton-afanasyev wrote:
> > > > ABataev wrote:
> > > > > anton-afanasyev wrote:
> > > > > > ABataev wrote:
> > > > > > > Better to modify `Bundle` parameter in `newTreeEntry` function, something like `Optional<std::pair<EntryState, Bundle>>` and pass `EntryState` directly at function call. You can implement it as a separate NFC patch.
> > > > > > Ok, I can fix it in a separate patch. But now I doubt that initial memory optimization with `IsScatteredOpds` elimination was worse than this final `EntryState` expansion requiring changes for all `newTreeEntry()` calls.
> > > > > Yes, it still will be better. Here you will allocate the value on the stack, which will be cleared after function termination. With an extra data member each TreeEntry will retain it till the end of life of the whole tree.
> > > > Ok
> > > Looks like you still do not pass the vectorization state as an argument. I thought we agreed to modify `Bundle` related param to be an `Optional` of EntryState and Bundle types? Better to create the entry with the correctly set inner state rather than modify them outside of the class. It breaks encapsulation.
> > Sorry, missed this comment. Yes, we agreed to do this, but as a separate NFC patch.
> Yes, thanks. Could you prepare this NFC patch before commiting this patch?
Hmm, I've decided to get rid of scary `make_pair` inside all `newTreeEntry()` callings, leaving most of the code untouched. So, added optional `EntryState` param to `newTreeEntry()` function. Minimal changes, so no need for separate patch. Is it good now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90445