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

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1754
+                          TreeEntry::EntryState State = TreeEntry::Vectorize) {
     bool Vectorized = (bool)Bundle;
     VectorizableTree.push_back(std::make_unique<TreeEntry>(VectorizableTree));
----------------
ABataev wrote:
> anton-afanasyev wrote:
> > ABataev wrote:
> > > It may conflict with `State` param value and may lead to incorrect undrstanding/decisions in future. That's why I proposed to merge `EntryState` and `Bundle` into one `Optional<std::pair<>>`, though this solution also is not quite optimal.
> > > Maybe better to split this function into 2 overloaded copies, one for gather and one for vectorized state?
> > Would it be better to leave all as it was before and just add `setEntryState()` method to accurately set `ScatterVectorize` (or other state in future)?
> Nah, it is similar to what was before. Better to set the correct state when constructing the state rather than construct it with the incorrect flag and then trying to fix it on-the-fly. It is bad design decision and always a source of bugs.
Ok, thanks! So I created overloaded copy of `newTreeEntry()` especially for `EntryState` given explicitly. Is it good now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90445