This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SME] Add load/store intrinsics
ClosedPublic

Authored by RosieSumpter on Jun 7 2022, 5:39 AM.

Details

Summary

This patch adds implementations for the load/store SME ACLE intrinsics:

  • @llvm.aarch64.sme.ld1*
  • @llvm.aarch64.sme.st1*

Diff Detail

Event Timeline

RosieSumpter created this revision.Jun 7 2022, 5:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 5:39 AM
RosieSumpter requested review of this revision.Jun 7 2022, 5:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 5:39 AM
paulwalker-arm added inline comments.
llvm/include/llvm/IR/IntrinsicsAArch64.td
2603

Given the transition to opaque pointers, should we be using such specific pointer types or can we just use llvm_ptr_ty instead?

I'm not going to comment on the design / implementation since I wrote much of it, but I've left a few minor comments

llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
343–373

how about:

for (MCSubRegIterator SubReg(AArch64::ZA, this, /*self=*/true);
     SubReg.isValid(); ++SubReg)
  Reserved.set(*SubReg);
llvm/lib/Target/AArch64/SMEInstrFormats.td
335

nit: indent

417

nit: fix formatting

476

nit: indent

david-arm added inline comments.Jun 8 2022, 2:54 AM
llvm/include/llvm/IR/IntrinsicsAArch64.td
2603

Is that the same thing as an opaque pointer though? Looking at one example here:

def int_aarch64_ldxp : Intrinsic<[llvm_i64_ty, llvm_i64_ty], [llvm_ptr_ty],
                                 [IntrNoFree, IntrWillReturn]>;

it seems like it just means a i8* pointer, judging by the codegen tests in CodeGen/AArch64/arm64-ldxr-stxr.ll:

declare %0 @llvm.aarch64.ldxp(i8*) nounwind

It's also implied in the definition:

Intrinsics.td:def llvm_ptr_ty        : LLVMPointerType<llvm_i8_ty>;             // i8*
paulwalker-arm added inline comments.Jun 8 2022, 5:20 AM
llvm/include/llvm/IR/IntrinsicsAArch64.td
2603

In truth I don't know. I did look to see if there was an opaque pointer def before posting my original comment but couldn't find anything. So I'm assuming that in the future llvm_ptr_ty will be the canonical representation for a pointer (given it's name). For these intrinsics the important point is that we don't attach any meaning to the pointer operands and thus llvm_ptr_ty seems like the better fit?

aemerson added inline comments.
llvm/include/llvm/IR/IntrinsicsAArch64.td
2603

@t.p.northover What do you think? Are typed pointers obsolete now in tablegen?

RosieSumpter marked 4 inline comments as done.
  • Addressed comments from @c-rhodes
  • Will update the patch if needed when there is confirmation about the use of typed pointers
aemerson accepted this revision.Jun 13 2022, 11:23 AM

We can revisit the opaque pointers issue later. LGTM otherwise with a nit.

llvm/lib/Target/AArch64/AArch64RegisterInfo.td
1215

Hoist these to the outside so you only do let isAllocatable = 0 in { once?

This revision is now accepted and ready to land.Jun 13 2022, 11:23 AM
This revision was automatically updated to reflect the committed changes.
RosieSumpter marked an inline comment as done.
RosieSumpter added inline comments.Jun 14 2022, 3:19 AM
llvm/lib/Target/AArch64/AArch64RegisterInfo.td
1215

Thanks for reviewing @aemerson, I made this change before committing.