LGTM, thanks!

LGTM

Address comment

Address comments

Add testcase (command crashed before)

Hi Alexey! Is this patch ready for reviewing or will other patches be splitted from this one?

In D98714#2764135, @vdmitrie wrote:

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

$opt -S -slp-vectorizer -slp-threshold=-10000 test/Transforms/SLPVectorizer/X86/insert-element-build-vector.ll -slp-min-tree-size=0
opt: .../llvm-project/llvm/lib/IR/Type.cpp:648: static llvm::FixedVectorType* llvm::FixedVectorType::get(llvm::Type*, unsigned int): Assertion `isValidElementType(ElementType) && "Element type of a VectorType must " "be an integer, floating point, or " "pointer type."' failed.

Thanks,

Valery

In D98714#2758640, @rupprecht wrote:

This patch introduces an assertion error we believe may be contributing to a miscompile (along with some other recent SLP patches -- this patch fixes the reduced case in http://llvm.org/PR50323, but doesn't fix the full case it was reduced from):

Address a nit

@RKSimon, sure, rebased

Add FIXME for non-consecutive insert case

Address comments

Address comments (minor)

Address comments, refactor

Please add reference to summary to previous patch related to this one: https://reviews.llvm.org/D100495. Other looks good.

Fixed comment

Addressed new comments

Fix names, minors

Reused getOperandIndex() for InsertElement instructions

Addressed comments

Addressed all comments

Ok, looks good.

In D101297#2719689, @ABataev wrote:

Yes, I believe this will fix PR50015, though it doesn't fix the whole problem related to lowering.

No, I don't think it will fix PR50015, it may hide it in some cases, yes. My main goal is to improve vectorization in general. Ok, I will rework it and land it in in a cost model for masked gathers.

I believe it will work correct after lowering fixes. I'm still investigating this case https://bugs.llvm.org/show_bug.cgi?id=50015, this is definitly lowering issue since llvm code looks correct.
Ok, I think this patch is good temp solution until miscompile issue isn't solved.

Maybe better to rework it and land it in X86TargetTransfromInfo.cpp for better cost modelling if it is not legal directly?

Yes, I believe this will fix PR50015, though it doesn't fix the whole problem related to lowering.

Is there a bugreport with reproducer?

In D101297#2719529, @ABataev wrote:

In D101297#2719521, @anton-afanasyev wrote:

Pure cost won't work here, we need to check if this is legal just to try to generate masked gather at all. Cost problem is different.

Can't we rely on cost model to deal with this? This can be profitable for futher vectorization, when we can start from such gather loads as seed. Yes, we have "unlegal" gathers for llvm ir, but they are lowered to legal code eventually. What is the motivation for this change?

The cost model must be tweaked to support this kind of lowering. I can rework the patch in terms of cost model improvement to handle the case with non-legal masked gather more correctly instead of direct legality checks in SLP. Will it work?

Pure cost won't work here, we need to check if this is legal just to try to generate masked gather at all. Cost problem is different.

Address some comments

Remake. Get rid of inserts reordering.

Addressed comment

Fix minors

Make use of ReorderIndices instead of sorting ScheduleData's

Refactored code, removed new TreeEntry::State. Make InsertElementInst passing through ordinary scheduling, allowing inner deps for bundle of insertelements.

Refactored code, removed new TreeEntry::State. Make InsertElementInst passing through ordinary scheduling, allowing inner deps for bundle of insertelements.

Closed by commit: https://reviews.llvm.org/rG9abe50047330
(modified summary a bit to comply with changes)

LGTM except for the comment. I think MaxVecRegSize % EltSize != 0 check could be removed (together with comment).

Closed by commit https://reviews.llvm.org/rGdd90c36d601e

Sorry, prematurely closed this revision by mistake.

In D98596#2629238, @bule wrote:

In D98596#2629211, @anton-afanasyev wrote:

In D98596#2629181, @bule wrote:

Fix the duplicate element error that cause the failure in CI

Ok, is it working now? I could commit it on your behalf.

Thanks a lot, I am waiting for the build status. Does it still need anton @anton-afanasyev to review it first? since I have changed the code a lot.

In D98596#2629181, @bule wrote:

Fix the duplicate element error that cause the failure in CI

The same work is resumed here: D98714, so abandoning this.

Here is another fix approach: https://reviews.llvm.org/D98714

LGTM

I'm trying another approach here, so abandoned this for a while.

Also could you please precommit test or just make diff against it (to see test changes before/after patch)?

LGTM

LG after addressing all comments.

Actually, it is not reducing. This is how test-suite python script works. So, here lhs - number of instructions after this patch, rhs - before. And the less relative number, the more vector instructions we actually generate.

In D57059#2555233, @ABataev wrote:

This is an integral patch, going to split it into several smaller patches.

Btw, how could it be explained NumVectorInstructions stat reducing after this patch?