[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:
> > > > 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?
> Ah let me check. I guess you mean `-flto` right?
Yes. There were regressions because of too early phi nodes vectorization at compile time, which prevents some later optimizations at link time. We could fix this by checking if LTO is enabled and the stage is linking. In this case we can set the flag to true for phis and other nodes. It would be good if you could make a patch for this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150288