This is an archive of the discontinued LLVM Phabricator instance.

[LV] Support Interleaved Store Group With Gaps
ClosedPublic

Authored by dorit on Jun 22 2021, 2:35 PM.

Details

Summary

Teach LV to use masked-store to support interleave-store-group with gaps (instead of scatters/scalarization).

The symmetric case of using masked-load to support interleaved-load-group with gaps was introduced a while ago, by https://reviews.llvm.org/D53668;
This patch completes the store-scenario leftover from D53668, and solves PR50566.

Diff Detail

Event Timeline

dorit created this revision.Jun 22 2021, 2:35 PM
dorit requested review of this revision.Jun 22 2021, 2:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2021, 2:35 PM

Costmodel is wrong: if there are gaps, you need to load the vector of original values, and insert the non-gap elements into it.

dorit added a comment.Jun 22 2021, 2:48 PM

Costmodel is wrong: if there are gaps, you need to load the vector of original values, and insert the non-gap elements into it.

but we are using a masked-store, the non-gaps will be masked-out, they are undefs...

Costmodel is wrong: if there are gaps, you need to load the vector of original values, and insert the non-gap elements into it.

but we are using a masked-store, the non-gaps will be masked-out, they are undefs...

Yep, didn't see this is only for when masked stores are used/allowed.

dorit updated this revision to Diff 353859.Jun 23 2021, 12:22 AM

updated formatting

dorit updated this revision to Diff 353861.Jun 23 2021, 12:31 AM

(accidentally uploaded without context)

dorit added a reviewer: gilr.Jun 23 2021, 3:19 AM
Ayal added a comment.Jun 24 2021, 1:04 AM

Thanks for following up on D53668 (with a gap ;-)! Looks good to me, added some minor comments, mostly about comments.

llvm/include/llvm/CodeGen/BasicTTIImpl.h
1178

The above comment should be updated, along with its consequences...

In some cases, scaling might be able to handle gaps w/o masking, for both loads and stores; perhaps worth leaving behind a TODO.

1230

Update this example to also include gaps, similar to the interleaved load with gaps example above?

llvm/lib/Analysis/VectorUtils.cpp
1276

typo: specualtive

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2831

"scalable vectors not yet supported." >>
"masking gaps for scalable vectors is not supported yet."

llvm/test/Transforms/LoopVectorize/X86/x86-interleaved-store-accesses-with-gaps.ll
18

Can also relate this test case (or file) to PR50566.

dorit updated this revision to Diff 354236.Jun 24 2021, 6:31 AM

Addressed review comments.

dorit marked 5 inline comments as done.Jun 24 2021, 6:33 AM

Thanks, Ayal! Incorporated all your comments.

Ayal accepted this revision.Jun 25 2021, 1:01 PM

This looks good to me, thanks!
Adding a minor final optional nit.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5415

nit: may want to update this comment to explain of three reasons, as they appear below, further elaborating the second reason.

This revision is now accepted and ready to land.Jun 25 2021, 1:01 PM
dorit updated this revision to Diff 354744.Jun 27 2021, 7:02 AM

updated a comment, as pointed out in the review

dorit added a comment.Jun 28 2021, 3:18 AM

Will get back to pushing this patch in a few weeks (unfortunately didn't make it before going away on vacation).

dorit updated this revision to Diff 364428.Aug 5 2021, 5:31 AM

rebased

dorit updated this revision to Diff 365002.Aug 7 2021, 11:46 PM

Addressed tidy warnings

dorit updated this revision to Diff 365004.Aug 8 2021, 12:35 AM

addressed clang-format issue

This revision was automatically updated to reflect the committed changes.