[PATCH] D150288: [SLP] Simplify `tryToVectorizeSequence()`

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:14276
-    function_ref<bool(ArrayRef<T *>, bool)> TryToVectorizeHelper,
-    bool MaxVFOnly, BoUpSLP &R) {
   bool Changed = false;
----------------
ABataev wrote:
> vporpo wrote:
> > ABataev wrote:
> > > vporpo wrote:
> > > > ABataev wrote:
> > > > > Removal of this parameter causes regressions in the vectorization, better to restore it.
> > > > Do you have some examples where this is needed?
> > > No, I don't, just from my experience when I added this parameter. Testing showed regressions in LTO mode with it, IIRC.
> > Is this in SPEC?
> IIRC, test/Transforms/SLPVectorizer/slp-max-phi-size.ll is one of the tests
Yes


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:14278-14280
+  MapVector<Type *, SmallVector<T *>> SeedsMap;
+  for (T *Seed : UnsortedSeeds)
+    SeedsMap[Seed->getType()].push_back(Seed);
----------------
vporpo wrote:
> ABataev wrote:
> > vporpo wrote:
> > > ABataev wrote:
> > > > It increases memory consumption.
> > > The previous version was using a `Candidates` vector, this one is using a map, so the memory consumption should be fairly similar.
> > > Also this is not a global data structure, it is being freed right away, so memory consumption shouldn't matter much in this context.
> > It is not quite map, it is mapvector, which is vector + map
> OK, so if I use an std::map would you be fine with this?
DenseMap should be good here, yes, since the order of the types does not matter


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150288