This is an archive of the discontinued LLVM Phabricator instance.

[SLP] Use the width of value truncated just before storing
ClosedPublic

Authored by anton-afanasyev on Dec 8 2020, 1:59 AM.

Details

Summary

For stores chain vectorization we choose the size of vector
elements to ensure we fit to minimum and maximum vector register
size for the number of elements given. This patch corrects vector
element size choosing the width of value truncated just before
storing instead of the width of value stored.

Fixes PR46983

Diff Detail

Event Timeline

anton-afanasyev created this revision.Dec 8 2020, 1:59 AM
anton-afanasyev requested review of this revision.Dec 8 2020, 1:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2020, 1:59 AM

Thanks!

@RKSimon?

llvm/test/Transforms/SLPVectorizer/X86/pr46983.ll
106–107

Wondering why no vectorization with avx..

anton-afanasyev marked an inline comment as done.Dec 8 2020, 2:12 AM
anton-afanasyev added inline comments.
llvm/test/Transforms/SLPVectorizer/X86/pr46983.ll
106–107

I've explained it here: https://bugs.llvm.org/show_bug.cgi?id=46983#c5. Actually this case is not related to truncating stores issue and actually is not a bug: we have a high cost for v4i64 type for some archs (see https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/X86/X86TargetTransformInfo.cpp#L808 and https://github.com/llvm/llvm-project/blob/master/llvm/test/Transforms/SLPVectorizer/X86/pr46983.ll#L165).

@anton-afanasyev Please can you rebase? I added extra test coverage in rG41d0666391131ddee451085c72ba6513872e7f6c

@anton-afanasyev Please can you rebase? I added extra test coverage in rG41d0666391131ddee451085c72ba6513872e7f6c

Rebased, but the diff is the same.

RKSimon added inline comments.Dec 8 2020, 6:41 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
5547

Does InstrElementSize have to updated at all?

anton-afanasyev added inline comments.Dec 8 2020, 7:06 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
5547

Do you mean to remove its using at all? This is just a cache for sizes needed not to traverse the expression tree multiple times.

RKSimon accepted this revision.Dec 8 2020, 7:57 AM

LGTM - cheers

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
5547

Ah - thanks for confirming.

This revision is now accepted and ready to land.Dec 8 2020, 7:57 AM
This revision was landed with ongoing or failed builds.Dec 9 2020, 5:39 AM
This revision was automatically updated to reflect the committed changes.