This patch adds the register + register addressing mode for
SVE contiguous load and store intrinsics (LD1 & ST1)
Details
Diff Detail
Event Timeline
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. |
I think I made a mess with the Actions for this review! I mean to accept it, not to enforce the nit comments!
Francesco
- Renamed ld1nf multiclass to ldnf1
- Split out existing reg+imm tests into their own files
- Renamed 'offset' to 'index' in reg+reg tests
The instruction mnemonic is ldfn1*, maybe we should call this multiclass ldnf1 instead of ld1nf?