This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][NFC] Reuse getDeinterleaveViaVNSRL to lower deinterleave intrinsics
ClosedPublic

Authored by luke on Feb 22 2023, 11:32 AM.

Details

Summary

This modifies it to work on both scalable and fixed vectors

Diff Detail

Event Timeline

luke created this revision.Feb 22 2023, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2023, 11:32 AM
luke requested review of this revision.Feb 22 2023, 11:32 AM
luke updated this revision to Diff 499609.Feb 22 2023, 11:40 AM

Label as NFC

luke retitled this revision from [RISCV] Reuse getDeinterleaveViaVNSRL to lower deinterleave intrinsics to [RISCV][NFC] Reuse getDeinterleaveViaVNSRL to lower deinterleave intrinsics.Feb 22 2023, 11:40 AM
reames added inline comments.Feb 22 2023, 1:21 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2950

Personally, I'd leave this duplicated in each caller. Keeping the helper as an unconditionally successful routine seems worth the very minor code duplication.

craig.topper added inline comments.Feb 22 2023, 2:49 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3163

Can this nest in the if (ContainerVT.isFixedLengthVector()) earlier. The source and destination should alway be both fixed or both scalable.

3167

To do my above suggestion, this would need to use ContainerVT to keep SrcContainerVT isolated to the body of the if (ContainerVT.isFixedLengthVector())

luke updated this revision to Diff 499793.Feb 23 2023, 3:30 AM
luke marked an inline comment as done.

Address comments

luke marked 2 inline comments as done.Feb 23 2023, 3:30 AM
reames accepted this revision.Feb 23 2023, 7:23 AM

LGTM w/comment applied.

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

There's no point in moving this check to the caller as the isDeinterleaveShuffle routine is only being used here. (Unless maybe you have another patch in mind, but if so, the change should probably be it's own NFC with that explained.)

This revision is now accepted and ready to land.Feb 23 2023, 7:23 AM
luke added inline comments.Feb 23 2023, 8:00 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3478

Yeah, IMO checking if the type is widenable is orthogonal to checking if a vector shuffle is a deinterleave. Splitting it out lets us reuse it for combining deinterleaves of loads of fixed vectors

reames added inline comments.Feb 23 2023, 8:01 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3478

Please revert to the old form of the check, and we can discuss this change in the DAG combine change which uses it.

luke updated this revision to Diff 499859.Feb 23 2023, 8:13 AM

Leave isDeinterleaveShuffle intact

luke marked 2 inline comments as done.Feb 23 2023, 8:14 AM
luke updated this revision to Diff 499862.Feb 23 2023, 8:17 AM

Update commit message

luke edited the summary of this revision. (Show Details)Feb 23 2023, 8:17 AM
This revision was landed with ongoing or failed builds.Feb 23 2023, 8:23 AM
This revision was automatically updated to reflect the committed changes.