This is an archive of the discontinued LLVM Phabricator instance.

[SLP] Fix "gathering" of vector values
ClosedPublic

Authored by anton-afanasyev on May 18 2021, 2:05 AM.

Details

Summary

For rare exceptional case vector tree node (insertelements for now only)
is marked as NeedToGather, this case is processed by patch. Follow-up
of D98714 to fix bug reported here https://reviews.llvm.org/D98714#2764135.

Diff Detail

Event Timeline

anton-afanasyev requested review of this revision.May 18 2021, 2:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2021, 2:05 AM

Add testcase?

Add testcase (command crashed before)

ABataev added inline comments.May 19 2021, 5:39 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3697–3699

Not sure we should return 0 here, maybe better to return some max cost, like INT_MAX?

4706–4708

Is this correct? Need to return the last insertelement instruction in the sequence

llvm/test/Transforms/SLPVectorizer/X86/insert-element-build-vector-inseltpoison.ll
45–80

Why the check lines are removed?

anton-afanasyev marked 3 inline comments as done.May 22 2021, 12:35 PM
anton-afanasyev added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3697–3699

There is no difference for now (the only one is compile time). We could also just prohibit marking NeedToGather for vector node (it could be done while checking for ephemeral values only).

But I've done it this way since it looks more transparent and also we could use zero cost for the future cases when/if such NeedToGather vector node is not the only member of tree and whole tree vectorization is beneficial.

4706–4708

Sure, fixed!

llvm/test/Transforms/SLPVectorizer/X86/insert-element-build-vector-inseltpoison.ll
45–80

Fixed

anton-afanasyev marked 3 inline comments as done.

Address comments

ABataev added inline comments.May 24 2021, 4:56 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3697–3699

I think it is better to use TTI::TCC_Expensive to mark it explicitly as an expensive operation. Or even invalid cost, if possible.

Address comment

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3697–3699

Ok, used Invalid cost.
Deleted gathering code, since gathering isn't supposed now.

ABataev added inline comments.May 24 2021, 6:43 AM
llvm/test/Transforms/SLPVectorizer/X86/insert-element-build-vector-inseltpoison.ll
149–156

Why are the instructions reordered if no vectorization should happen?

anton-afanasyev marked an inline comment as done.May 24 2021, 7:54 AM
anton-afanasyev added inline comments.
llvm/test/Transforms/SLPVectorizer/X86/insert-element-build-vector-inseltpoison.ll
149–156

Vectorization of inserts is prohibited now, yes, but compiler still "vectorizes" small trees of size 1 with any valid cost due to options -slp-threshold=-10000 -slp-min-tree-size=0. This is not related to this patch itself.

This revision is now accepted and ready to land.May 24 2021, 7:56 AM
This revision was landed with ongoing or failed builds.May 24 2021, 3:36 PM
This revision was automatically updated to reflect the committed changes.
anton-afanasyev marked an inline comment as done.