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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/test/CodeGen/AArch64/ldN-reg-imm-alloca.ll:14
+; CHECK-NEXT:    ret
+  %alloc = alloca [64 x i8], i32 1, align 16
+  %ptr = bitcast [64 x i8]* %alloc to i8*
----------------
Looking at the code-changes, I actually don't think that the fixed-width property (for the alloca and vscale_range) matters for these tests. I think you only really need the scalable tests.


================
Comment at: llvm/test/CodeGen/AArch64/ldN-reg-imm-alloca.ll:96
+; CHECK-NEXT:    addvl x8, sp, #1
+; CHECK-NEXT:    ld2h { z0.h, z1.h }, p0/z, [x8, #2, mul vl]
+; CHECK-NEXT:    addvl sp, sp, #5
----------------
Can you make sure that all these tests have a positive test for the maximum range, and a negative test for one-beyond the maximum range ?


================
Comment at: llvm/test/CodeGen/AArch64/ldN-reg-imm-alloca.ll:200-201
+; CHECK-NEXT:    addvl sp, sp, #-13
+; CHECK-NEXT:    .cfi_escape 0x0f, 0x0d, 0x8f, 0x00, 0x11, 0x10, 0x22, 0x11, 0xe8, 0x00, 0x92, 0x2e, 0x00, 0x1e, 0x22 // sp + 16 + 104 * VG
+; CHECK-NEXT:    .cfi_offset w29, -16
+; CHECK-NEXT:    addvl x8, sp, #1
----------------
nit: I'd recommend adding `nounwind` attribute to avoid the CFI info, as it's not relevant to the test.


================
Comment at: llvm/test/CodeGen/AArch64/ldN-reg-imm-alloca.ll:317
+  %alloca1 = alloca <vscale x 2 x double>, i32 13
+  %alloca2 = alloca <vscale x 4 x i32>, i32 2
+  %base = getelementptr <vscale x 2 x double>, <vscale x 2 x double>* %alloca1, i64 9, i64 0
----------------
alloca2 is unused? (also true for other cases)


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-ld2-alloca.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -aarch64-sve-vector-bits-min=256 < %s | FileCheck %s
----------------
david-arm wrote:
> nit: I think maybe we don't need this file anymore, since all the various loads/stores are now covered in the other tests?
Perhaps we can keep this one test if the other fixed-width tests are removed, just to ensure we cover that case too (even if it doesn't necessarily exercise anything specific in this patch that isn't already tested otherwise). If so, then I'd recommend expressing it with an explicit st2 intrinsic instead of a shufflevector+store.


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-ld2-alloca.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -aarch64-sve-vector-bits-min=256 < %s | FileCheck %s
+
----------------
redundant if vscale_range is set.


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

https://reviews.llvm.org/D119338