This is an archive of the discontinued LLVM Phabricator instance.

[VP] Vector predicated vector splice intrinsic
ClosedPublic

Authored by vkmr on Jun 8 2021, 7:46 AM.

Details

Summary

This patch introduces the vector-predicated version of the experimental_vector_splice intrinsic [1] at the IR level. It considers the active vector length for both vectors and and uses a vector mask to disable certain lanes in the result.

[1] https://reviews.llvm.org/D94708

Diff Detail

Event Timeline

vkmr created this revision.Jun 8 2021, 7:46 AM
vkmr requested review of this revision.Jun 8 2021, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2021, 7:46 AM
vkmr retitled this revision from [VP] Length predicated vector splice intrinsic to [VP] Vector predicated vector splice intrinsic.Jun 8 2021, 8:01 AM

As discussed on the call, maybe rename llvm.experimental.vp.vector.splice to llvm.experimental.vp.splice.. the "vector" is implied

Are evl1 and evl2 likely to be the same value? Do you have an example when they would be different?

vkmr added a comment.Jun 10 2021, 6:51 AM

Are evl1 and evl2 likely to be the same value? Do you have an example when they would be different?

For a first order reduction, using a tail-folded loop with VP, if the EVL is computed using target-specific instructions, there is no guarantee that the EVL for the previous iteration would be the same as the EVL for current iteration or even that it would be equal to the runtime VF. For a more specific example, for RISC-V, the constraints on the vset{i}vl{i} instruction allow the EVL for the last non-tail iteration to be less than the runtime VF and unequal to the EVL for the tail iteration.

This patch needs to implement VPIntrinsic::getDeclarationForParams.

vkmr updated this revision to Diff 356085.Jul 1 2021, 7:27 PM

Addressed @simoll's comment. Took the liberty to clang-format part of the test for memory ops.

vkmr added a comment.Jul 1 2021, 7:30 PM

This patch needs to implement VPIntrinsic::getDeclarationForParams.

for vp_splice, the implementation will be the same as the default case so I did not implement it explicitly. Did I miss anything?

vkmr added a comment.Jul 1 2021, 7:31 PM

As discussed on the call, maybe rename llvm.experimental.vp.vector.splice to llvm.experimental.vp.splice.. the "vector" is implied

Done!

simoll added a comment.Jul 2 2021, 2:48 AM

This patch needs to implement VPIntrinsic::getDeclarationForParams.

for vp_splice, the implementation will be the same as the default case so I did not implement it explicitly. Did I miss anything?

You didn't. LGTM with nit.

llvm/docs/LangRef.rst
18675

I think it's not totally obvious that %imm has to be an immediate. Eg, you could add immarg in the intrinsic declarations (Syntax section) and mention it again in the Arguments section.

simoll added inline comments.Jul 2 2021, 3:58 AM
llvm/include/llvm/IR/VPIntrinsics.def
237

+1 what lint said

vkmr updated this revision to Diff 356477.Jul 5 2021, 4:47 AM

Addressed comments.

vkmr marked 2 inline comments as done.Jul 5 2021, 4:47 AM
Matt added a subscriber: Matt.Jul 12 2021, 7:17 AM
frasercrmck added inline comments.Jul 13 2021, 9:44 AM
llvm/docs/LangRef.rst
18688

typo: selectng

18691

I think undef is generally back-ticked in this document

vkmr updated this revision to Diff 366362.Aug 13 2021, 3:30 PM

addressed typos found by @frasercrmck.

vkmr marked 2 inline comments as done.Aug 13 2021, 3:30 PM
simoll accepted this revision.Aug 19 2021, 5:42 AM
This revision is now accepted and ready to land.Aug 19 2021, 5:42 AM
vkmr added a comment.Sep 28 2021, 6:10 AM

Can someone please commit it on my behalf? I don't have commit access.

This revision was automatically updated to reflect the committed changes.
vkmr added a comment.Oct 12 2021, 7:14 AM

@simoll I believe there is a way to commit without changing the author, such that both phabricator and github show something like "authored by <author> and committed by <committer>".

@simoll I believe there is a way to commit without changing the author, such that both phabricator and github show something like "authored by <author> and committed by <committer>".

That's what i normally do. There were some issues with arcanist and i had to commit manually.