This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Lower interleave2 intrinsics to vsseg2
ClosedPublic

Authored by luke on Jun 27 2023, 5:52 AM.

Details

Summary

This patch teaches the RISCV TargetLowering class to lower interleave
intrinsics to vsseg2, so it can lower interleaved stores for scalable vectors.
Previously, we could only lower stores of interleaves for fixed length vectors
with vector shuffles.

This uses the lowerInterleaveIntrinsic interface for the interleaved
access pass that was added in D146218, and subsumes the DAG combine
approach taken in D144175

Diff Detail

Event Timeline

luke created this revision.Jun 27 2023, 5:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2023, 5:52 AM
luke requested review of this revision.Jun 27 2023, 5:52 AM
luke updated this revision to Diff 534974.Jun 27 2023, 7:29 AM

Fix isLegalInterleavedAccessType check

craig.topper added inline comments.Jun 27 2023, 9:59 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
16914

While VLMaxSentinel happens to be -1, I think that was more of a contract between isel and RISCVInsertVSETVLI for the MachineOperand. Should probably just use Constant::getAllOnes here.

luke updated this revision to Diff 535043.Jun 27 2023, 10:19 AM

Use Constant::getAllOnesValue instead of VLMaxSentinel

craig.topper added inline comments.Jun 27 2023, 9:19 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
16726

remove "fixed length" from this comment?

luke updated this revision to Diff 535302.Jun 28 2023, 2:45 AM

Address comment and refactor isLegalInterleavedAccessType a bit

luke marked 2 inline comments as done.Jun 28 2023, 2:48 AM
reames accepted this revision.Jul 5 2023, 9:35 AM

LGTM

I'm fine with the alignment piece being done after this lands. It is a bug, but it's a bug we already have and should be very narrow. Just make sure you tackle that in the next few days.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
16881

Please add an assert that SI isSimple. This is checked by the caller, but this could relies on it for correctness, so we should guard against future changes.

16892

There's a missing legality check here. It's actually missing in the lowerInterleavedStore as well.

If the store is misaligned (i.e. has an explicit align 1), then the resulting segmented store may not be legal. For a processor which supports misaligned scalars, but not misaligned vectors (or merely not misaligned segment stores), this could be a problem.

Probably the best fix is to add an alignment parameter to isLegalInterleaveAccessType.

This revision is now accepted and ready to land.Jul 5 2023, 9:35 AM
This revision was landed with ongoing or failed builds.Jul 5 2023, 11:24 AM
This revision was automatically updated to reflect the committed changes.