Hi, there is another issue that can be reproduced with existing test case:

Hi Florian, I just came across this patch. Could you please follow up to update descriptions of some methods that you have updated with the patch?
I did not check the most recent LLVM project version, thus if you see that a comment is irrelevant anymore then disregard it.
Thank you.
Valery

It sounds like throttling patch should resolve this issue as cutting out ScatterVectorize entry with high cost will effectively return to previous behavior.

Current SLP has significant drawback with regard to its cost modeling. And this patch highlights it.
Consider we have four scalar loads of i8 type. With prior approach (vectorization overhead) we had cost for such entry 4 (x86 target).
With this new approach we have two entries instead of one: ScatterVectorize loads + NeedToGather GEPs. And costs for these entries are 6 and 10 respectively, thus cost increased from 4 to 16.
And the problem here is once we put this pattern into the tree it pulls cost up for the entire tree. If we have multiple such patterns over the tree their effect is magnified. These entries finally outweigh possible profit of vectorization for remaining portion of the tree and we end up not vectorizing it at all (even if downstream optimizations could probably change it into optimal code). If SLP could make choice vectorization overhead vs gather intrinsic based in their costs while building vectorizable tree the outcome could be different.

In D82227#2389212, @jonpa wrote:

I am afraid that this patch actually has a bad impact on performance on SystemZ, and unfortunately this was not known until now. It would be very appreciated if we could rework this and get back the old behaviour on SystemZ somehow...

See https://bugs.llvm.org/show_bug.cgi?id=48155

Vector combine dies on assertion with this test case.

replaced \param with \p

Thank you, Alexey. I addressed your comments in new upload.

Thank you for prompt review.
Updated patch to address your comments.

In D76956#1983984, @RKSimon wrote:

@vdmitrie I've created D78216 to demonstrate the approach I had in mind.

ping

Addressed comments.

clang-format

Updated getScalarizationOverhead to use new TTI API for chained inserts/extracts.
This reduced cost overestimates in many cases brought with https://reviews.llvm.org/D74976.
Cost model tests adjusted to reflect that.
Tests Transforms/SLPVectorizer/X86/resched.ll and Transforms/LoopVectorize/X86/strided_load_cost.ll reverted to state prior to D74976.

In D76956#1947796, @RKSimon wrote:

We already have getScalarizationOverhead - can't we extend that instead?

Fixed format and test failures.

SO, we have correct implementation and this is just a check to prevent possible bugs in the future?

In D75296#1896620, @ABataev wrote:

Maybe it is better to implement the two-steps approach? If it may lead to a crash, it will definitely cause a crash somewhere

Thanks for the reproducer. Does this one produce an incorrect result, crashes compiler or something else? What exactly?

There is still problem with extracts.
This case shows it:
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

Update: I have spent some time investigating unit tests infra and finally came to conclusion that it is not quite possible to create a unit test that calls BoUpSLP::dumpVectorizableTree() as we need to construct BoUpSLP object. We are unable to do that having class forward declaration only.

In D69812#1733047, @ABataev wrote:

Something like llvm/unittests/IR/DominatorTreeBatchUpdatesTest.cpp?

Do you have any ideas how a test might look?

unittests?

In D69812#1732782, @ABataev wrote:

Tests?

Abandoned as issue was fixed with D51748

In D51552#1228072, @tstellar wrote:

I've just updated the patch with some fixes, can you try again?

In D51552#1228027, @tstellar wrote:

What CMake arguments were you using?

cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Release -DLLVM_TARGETS_TO_BUILD="X86" -DCMAKE_INSTALL_PREFIX="/your/install/dir" "/local/ll.org/source/root/llvm"

In D51552#1227797, @tstellar wrote:

D51748 proposes refactoring this whole function and effectively makes the same change. Would you be able to test this to see if it helps your uses case?

In D51552#1227791, @rnk wrote:

I looked at the documentation for the cmake install command, but I still have no idea what a component is or what effect this option has. Can you elaborate on that?