[PATCH] D119338: [AArch64][SVE] Add structured load/store opcodes to getMemOpInfo

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Hi @kmclaughlin thanks for updating the tests to work only on scalable types. I think the tests are still a bit inconsistent, and I've tried to highlight some of the inconsistencies in one of the files. Perhaps iteratively updating this file will just make things more complicated, so maybe it's easier to start with a clean slate and test:

- ld2b valid min offset
- ld2b valid max offset
- ld2b one below min offset
- ld2b one beyond max offset

- ld2h valid min offset
- ld2h valid max offset
- ld2h one below min offset
- ld2h one beyond max offset

- ...

and doing that also for ld3, ld4, st2, st3 and st4.



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:2926
     break;
+
+  case AArch64::LD2B_IMM:
----------------
nit: remove redundant newlines here and below.


================
Comment at: llvm/test/CodeGen/AArch64/ldN-reg-imm-alloca.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+sve < %s | FileCheck %s
----------------
These files are all specific to SVE, can you prefix them with `sve-`?


================
Comment at: llvm/test/CodeGen/AArch64/ldN-reg-imm-alloca.ll:6
+
+define <vscale x 32 x i8> @ld2b_imm(<vscale x 16 x i1> %pg) vscale_range(2,2) nounwind {
+; CHECK-LABEL: ld2b_imm:
----------------
vscale range can now be removed.


================
Comment at: llvm/test/CodeGen/AArch64/ldN-reg-imm-alloca.ll:18
+  %ptr = bitcast <vscale x 32 x i8>* %alloc to i8*
+  store i8 zeroinitializer, i8* %ptr, align 16
+  %ld2 = call <vscale x 32 x i8> @llvm.aarch64.sve.ld2.nxv32i8(<vscale x 16 x i1> %pg, i8* %ptr)
----------------
What values do these stores add to these tests?


================
Comment at: llvm/test/CodeGen/AArch64/ldN-reg-imm-alloca.ll:23-38
+define <vscale x 16 x i16> @ld2h_imm(<vscale x 8 x i1> %pg) vscale_range(2,2) nounwind {
+; CHECK-LABEL: ld2h_imm:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    str x29, [sp, #-16]! // 8-byte Folded Spill
+; CHECK-NEXT:    addvl sp, sp, #-2
+; CHECK-NEXT:    strh wzr, [sp]
+; CHECK-NEXT:    ld2h { z0.h, z1.h }, p0/z, [sp]
----------------
personally I don't see a lot of value in tests where it's indexing into the alloca at offset 0.


================
Comment at: llvm/test/CodeGen/AArch64/ldN-reg-imm-alloca.ll:75
+; Test where the immediate is non-zero and in range
+define <vscale x 16 x half> @ld2b_f16_valid_imm(<vscale x 8 x i1> %pg) vscale_range(2,2) nounwind {
+; CHECK-LABEL: ld2b_f16_valid_imm:
----------------
`ld2b` for loading f16's? (i see the same in other tests)


================
Comment at: llvm/test/CodeGen/AArch64/ldN-reg-imm-alloca.ll:325
+; Test where the immediate is non-zero and in range
+define <vscale x 8 x double> @ld4b_f64_valid_imm(<vscale x 2 x i1> %pg) nounwind vscale_range(2,2) nounwind {
+; CHECK-LABEL: ld4b_f64_valid_imm:
----------------
I'm a bit confused why you're mixing tests for floating point and integer element types. We can just use only integer types, because loads/stores don't distinguish the types. It is not relevant for these tests since they're trying to test getMemOpInfo (as opposed to say, ISEL).


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

https://reviews.llvm.org/D119338