[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;
----------------
vporpo wrote:
> ABataev wrote:
> > vporpo wrote:
> > > ABataev wrote:
> > > > 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
> > > OK, I will run SPEC lto to check. 
> > > 
> > > > IIRC, test/Transforms/SLPVectorizer/slp-max-phi-size.ll is one of the tests
> > > Doesn't slp-max-phi-size.ll look OK though? As far as I understand the purpose of this test is to make sure that we don't vectorize wider than specified by the `-slp-max-vf` option.
> > Probably. If I do remember correctly, it caused regresssions because of too early optimization of phi nodes and some other instructions.
> I checked the C/C++ benchmarks of CPU2017 and I don't see any regressions. I also checked with our internal testing and there are no issues. So I think it is pretty safe. 
> In the unlikely case that it causes a regression, we can look into it again and add any missing tests. Wdyt?
Did you try LTO?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150288