[PATCH] D78509: [AArch64][SVE] Add addressing mode for contiguous loads & stores

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli requested changes to this revision.
fpetrogalli added a comment.
This revision now requires changes to proceed.

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



================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:1617
 
+  multiclass ld1nf<Instruction I, ValueType Ty, SDPatternOperator Load, ValueType PredTy, ValueType MemVT> {
+    // scalar + immediate (mul vl)
----------------
The instruction mnemonic is `ldfn1*`, maybe we should call this multiclass `ldnf1` instead of `ld1nf`?


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-ld1-addressing-mode-reg-reg.ll:7
+
+define <vscale x 16 x i8> @ld1b_i8(<vscale x 16 x i1> %pg, i8* %a, i64 %offset) {
+; CHECK-LABEL: ld1b_i8
----------------
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.


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-ld1.ll:1
 ; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+sve < %s | FileCheck %s
 
----------------
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`


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-st1-addressing-mode-reg-reg.ll:7
+
+define void @st1b_i8(<vscale x 16 x i8> %data, <vscale x 16 x i1> %pred, i8* %a, i64 %offset) {
+; CHECK-LABEL: st1b_i8:
----------------
Nit: same here. For the sake of consistency with other tests, I think you could rename `%offset` to `%index`.


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-st1.ll:1
 ; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+sve < %s | FileCheck %s
 
----------------
Nit: Same for this test, I think it is worth splitting for the sake of consistency with other tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78509/new/

https://reviews.llvm.org/D78509