This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Add addressing mode for contiguous loads & stores
ClosedPublic

Authored by kmclaughlin on Apr 20 2020, 10:48 AM.

Details

Summary

This patch adds the register + register addressing mode for
SVE contiguous load and store intrinsics (LD1 & ST1)

Diff Detail

Event Timeline

kmclaughlin created this revision.Apr 20 2020, 10:48 AM
Herald added a project: Restricted Project. · View Herald Transcript
fpetrogalli requested changes to this revision.Apr 20 2020, 11:33 AM

Hi @kmclaughlin , thank you for working on this!

The patch LGTM, but please consider renaming the multiclass for the non faulting loads like I suggested. The rest of the feedback is mostly cosmetic changes!

Francesco

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1617

The instruction mnemonic is ldfn1*, maybe we should call this multiclass ldnf1 instead of ld1nf?

llvm/test/CodeGen/AArch64/sve-intrinsics-ld1-addressing-mode-reg-reg.ll
7

Nit: I think you should rename the variable %offset to %index all through this tests, as usually we intend offset to represent bytes, while index is for indexes of arrays, which are scaled.

llvm/test/CodeGen/AArch64/sve-intrinsics-ld1.ll
1

For the sake of consistency, I think it is worth splitting this test into two tests. One that tests the pattern for the value of offset of 0 (you can leave the current name for the file), and one that tests all valid and invalid values for the reg+imm addressing mode, and call the test sve-intrinsics-ld1-addressing-mode-reg-imm.ll

llvm/test/CodeGen/AArch64/sve-intrinsics-st1-addressing-mode-reg-reg.ll
7

Nit: same here. For the sake of consistency with other tests, I think you could rename %offset to %index.

llvm/test/CodeGen/AArch64/sve-intrinsics-st1.ll
1

Nit: Same for this test, I think it is worth splitting for the sake of consistency with other tests.

This revision now requires changes to proceed.Apr 20 2020, 11:33 AM
fpetrogalli accepted this revision.Apr 20 2020, 11:37 AM

I think I made a mess with the Actions for this review! I mean to accept it, not to enforce the nit comments!

Francesco

This revision is now accepted and ready to land.Apr 20 2020, 11:37 AM
kmclaughlin marked 5 inline comments as done.
  • Renamed ld1nf multiclass to ldnf1
  • Split out existing reg+imm tests into their own files
  • Renamed 'offset' to 'index' in reg+reg tests

Thanks for taking a look at this, @fpetrogalli!

This revision was automatically updated to reflect the committed changes.