This is an archive of the discontinued LLVM Phabricator instance.

[llvm][SVE] IR intrinsic for LD1RO.
ClosedPublic

Authored by fpetrogalli on May 28 2020, 9:59 AM.

Diff Detail

Event Timeline

fpetrogalli created this revision.May 28 2020, 9:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2020, 9:59 AM
fpetrogalli marked an inline comment as done.May 28 2020, 10:01 AM
fpetrogalli added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11910

Note: I am tempted to merge performLD1ROCombine and performLD1RQCombine in a single function, as they only differ in the ISD node being used to replace the intrinsic. Let me know if this makes sense, I'll update the patch.

sdesmalen added inline comments.May 28 2020, 10:23 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11910

Yes, that makes sense.

I merged the performLD1R[Q|O]Combine into a single function.

fpetrogalli marked an inline comment as done.May 28 2020, 6:30 PM
sdesmalen accepted this revision.May 29 2020, 1:49 AM

Some nits, but LGTM otherwise.

llvm/include/llvm/IR/IntrinsicsAArch64.td
1308

super nit: move this under ld1rq?

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1461

super nit: same suggestions as above.

llvm/lib/Target/AArch64/AArch64ISelLowering.h
251

super nit: move under ld1rq.

llvm/test/CodeGen/AArch64/sve-intrinsics-ld1ro.ll
5

perhaps somewhat unrelated, but I see that llvm/test/CodeGen/AArch64/sve-intrinsics-loads.ll contains the ld1rq tests. (Oddly enough that file also contains tests for ldnt1, but nothing else)
Maybe we can merge the ld1r* tests into one file?

This revision is now accepted and ready to land.May 29 2020, 1:49 AM
fpetrogalli marked 5 inline comments as done.

Address the super nits. :)

Add bfloat test case to ld1ro.

Remove the bfloat test case, as bfloat support will land
separately. This brings the patch at the same state when it was
approved, so there is no need to reapprove it, I will just submit it.

fpetrogalli added inline comments.Jun 3 2020, 6:37 AM
llvm/test/CodeGen/AArch64/sve-intrinsics-ld1ro.ll
5

I'd rather keep the ld1ro and ld1rq tests separate. I see more value in having more smaller test files than fewer big tests files, it make it easier to me to isolate implementation, reviews and possible failures.

This revision was automatically updated to reflect the committed changes.