goalc: arm64 vector loads and stores with a register offset were wrong (#4386)

Some neon tests, plus the fix they needed to run at all. `ldr q` and `str q` with a register offset had a bad option field so they don't decode. The s8 load was ignoring addr2 and reading from the base on its own. A few used post indexed forms, which move the address register. Rest of the avx list to come.
2026-08-22 23:11:40 -04:00 · 2026-08-22 14:00:13 -07:00
parent 8c926caa23
commit b59a6d1cf3
2 changed files with 182 additions and 34 deletions
@@ -2548,17 +2548,14 @@ InstructionARM64 loadvf_gpr64_plus_gpr64(Register dst, Register addr1, Register
  ASSERT(addr1 != addr2);
  ASSERT(addr1 != SP);
  ASSERT(addr2 != SP);
-  return InstructionARM64(Base(0b0011110011100000000010, 22), Rt(dst.id()), Rn(addr1.id()),
-                          Rm(addr1.id()));
+  return InstructionARM64(Base(0b0011110011100000111010, 22), Rt(dst.id()), Rn(addr1.id()),
+                          Rm(addr2.id()));
 }

 InstructionARM64 loadvf_gpr64_plus_gpr64_plus_s8(Register dst,
                                                 Register addr1,
                                                 Register addr2,
                                                 s64 offset) {
-  // https://www.scs.stanford.edu/~zyedidia/arm64/ldr_imm_fpsimd.html
-  // 128-bit variant
-  // LDR <Qt>, [<Xn|SP>], #<simm>
  ASSERT(dst.is_128bit_simd(instr_set));
  ASSERT(addr1.is_gpr(instr_set));
  ASSERT(addr2.is_gpr(instr_set));
@@ -2566,8 +2563,16 @@ InstructionARM64 loadvf_gpr64_plus_gpr64_plus_s8(Register dst,
  ASSERT(addr1 != SP);
  ASSERT(addr2 != SP);
  ASSERT(offset >= INT8_MIN && offset <= INT8_MAX);
-  return InstructionARM64(Base(0b0011110011000000000001, 22), Rt(dst.id()), Rn(addr1.id()),
-                          Imm9s(offset));
+  // base + index in x16, then ldur. imm9 covers the whole s8 range, so the offset folds in
+  // and this needs no second add.
+  return InstructionARM64(
+      {// https://www.scs.stanford.edu/~zyedidia/arm64/add_addsub_shift.html
+       // ADD <Xd>, <Xn>, <Xm>{, <shift> #<amount>}
+       InstructionARM64(Base(0b10001011000, 11), Rd(X16), Imm6(0), Rn(addr1.id()), Rm(addr2.id())),
+       // https://www.scs.stanford.edu/~zyedidia/arm64/ldur_fpsimd.html
+       // LDUR <Qt>, [<Xn|SP>{, #<simm>}]
+       // checked against clang, ldur q7, [x19, #124] is 3cc7c267
+       InstructionARM64(Base(0b0011110011000000000000, 22), Rt(dst.id()), Rn(X16), Imm9s(offset))});
 }

 InstructionARM64 loadvf_gpr64_plus_gpr64_plus_s32(Register dst,
@@ -2596,11 +2601,12 @@ InstructionARM64 loadvf_gpr64_plus_gpr64_plus_s32(Register dst,
    const auto add_instrs = construct_multiple_imm12_adds(offset, X16);
    instrs.insert(instrs.end(), add_instrs.begin(), add_instrs.end());
  }
-  // https://www.scs.stanford.edu/~zyedidia/arm64/ldr_imm_fpsimd.html
-  // 128-bit variant
-  // LDR <Qt>, [<Xn|SP>], #<simm>
+  // https://www.scs.stanford.edu/~zyedidia/arm64/ldur_fpsimd.html
+  // LDUR <Qt>, [<Xn|SP>{, #<simm>}]
+  // the whole address is in x16 already, so the offset is 0. the post indexed LDR is a
+  // different instruction that also writes x16 back.
  instrs.emplace_back(
-      InstructionARM64(Base(0b0011110011000000000001, 22), Rt(dst.id()), Rn(X16), Imm9s(0)));
+      InstructionARM64(Base(0b0011110011000000000000, 22), Rt(dst.id()), Rn(X16), Imm9s(0)));
  return InstructionARM64(instrs);
 }

@@ -2613,7 +2619,7 @@ InstructionARM64 storevf_gpr64_plus_gpr64(Register value, Register addr1, Regist
  ASSERT(addr2 != SP);
  // https://www.scs.stanford.edu/~zyedidia/arm64/str_reg_fpsimd.html
  // STR <Qt>, [<Xn|SP>, (<Wm>|<Xm>){, <extend> {<amount>}}]
-  return InstructionARM64(Base(0b0011110010100000011010, 22), Rt(value.id()), Rn(addr1.id()),
+  return InstructionARM64(Base(0b0011110010100000111010, 22), Rt(value.id()), Rn(addr1.id()),
                          Rm(addr2.id()));
 }

@@ -2628,26 +2634,16 @@ InstructionARM64 storevf_gpr64_plus_gpr64_plus_s8(Register value,
  ASSERT(addr1 != SP);
  ASSERT(addr2 != SP);
  ASSERT(offset >= INT8_MIN && offset <= INT8_MAX);
-  // first establish the base+index+offset value in x16
-  std::vector<InstructionARM64> instrs = {
-      // https://www.scs.stanford.edu/~zyedidia/arm64/add_addsub_shift.html
-      // ADD <Xd>, <Xn>, <Xm>{, <shift> #<amount>}
-      InstructionARM64(Base(0b10001011000, 11), Rd(X16), Imm6(0), Rn(addr1.id()), Rm(addr2.id())),
-  };
-  if (offset < 0) {
-    // we'll subtract instead
-    offset = std::abs(offset);
-    const auto sub_instrs = construct_multiple_imm12_subs(offset, X16);
-    instrs.insert(instrs.end(), sub_instrs.begin(), sub_instrs.end());
-  } else {
-    const auto add_instrs = construct_multiple_imm12_adds(offset, X16);
-    instrs.insert(instrs.end(), add_instrs.begin(), add_instrs.end());
-  }
-  // https://www.scs.stanford.edu/~zyedidia/arm64/str_imm_fpsimd.html
-  // STR <Qt>, [<Xn|SP>], #<simm>
-  instrs.emplace_back(
-      InstructionARM64(Base(0b0011110010000000000000, 22), Rt(value.id()), Rn(X16), Imm9s(0)));
-  return InstructionARM64(instrs);
+  // base + index in x16, then stur. imm9 covers the whole s8 range, so the offset folds in.
+  return InstructionARM64(
+      {// https://www.scs.stanford.edu/~zyedidia/arm64/add_addsub_shift.html
+       // ADD <Xd>, <Xn>, <Xm>{, <shift> #<amount>}
+       InstructionARM64(Base(0b10001011000, 11), Rd(X16), Imm6(0), Rn(addr1.id()), Rm(addr2.id())),
+       // https://www.scs.stanford.edu/~zyedidia/arm64/stur_fpsimd.html
+       // STUR <Qt>, [<Xn|SP>{, #<simm>}]
+       // checked against clang, stur q7, [x16, #124] is 3c87c207
+       InstructionARM64(Base(0b0011110010000000000000, 22), Rt(value.id()), Rn(X16),
+                        Imm9s(offset))});
 }

 InstructionARM64 storevf_gpr64_plus_gpr64_plus_s32(Register value,
@@ -1,3 +1,6 @@
+#include <cstring>
+#include <vector>
+
 #include "goalc/emitter/CodeTester.h"
 #include "goalc/emitter/IGen.h"
 #include "gtest/gtest.h"
@@ -12,7 +15,8 @@ CodeTester create_tester(int code_capacity = 1024) {
  tester.init_code_buffer(code_capacity);
  return tester;
 }
-};  // namespace
+
+}  // namespace

 TEST(NEONEmitter, VF_NOP) {
  CodeTester tester = create_tester();
@@ -24,4 +28,152 @@ TEST(NEONEmitter, WAIT_VF) {
  CodeTester tester = create_tester();
  tester.emit(IGen::wait_vf(tester.generator()));
  EXPECT_EQ(tester.dump_to_hex_string(true), "1F2003D5");
-}
+}
+
+// 128-bit loads and stores with a register offset. GOAL addresses vector memory as a base
+// register plus an offset register, so every addressing form has to use both.
+//
+// These run the code they emit, so they only build on an arm64 host. emitter_util.cpp guards
+// the arm64 tests that go through it the same way.
+#ifdef __aarch64__
+
+namespace {
+constexpr int kGoalPtr = 4096;
+constexpr int kBufSize = 16384;
+
+struct VecMem {
+  std::vector<u8> buf;
+  VecMem() : buf(kBufSize, 0) {}
+  u8* base() { return buf.data(); }
+  u8* at(int off) { return buf.data() + off; }
+};
+
+// two 16 byte vector slots, back to back
+constexpr int kA = kGoalPtr;
+constexpr int kB = kGoalPtr + 16;
+// far enough from kA that a displaced store cannot land on the markers around it
+constexpr int kDst = kGoalPtr + 4096;
+
+// x0 = base, x1 = source offset, x2 = second offset, x3 = destination offset
+template <typename EmitOp>
+void run_mem(CodeTester& tester, VecMem& mem, EmitOp emit_op) {
+  tester.clear();
+  tester.emit_push_all_gprs(true);
+  emit_op(tester);
+  tester.emit_pop_all_gprs(true);
+  tester.emit_return();
+  tester.execute_ret<u64>((u64)mem.base(), kA, kB, kDst);
+}
+}  // namespace
+
+TEST(NEONEmitter, vector_load_store_register_offset_uses_both_registers) {
+  CodeTester tester = create_tester(2048);
+  VecMem mem;
+
+  const u32 want[4] = {0x11112222u, 0x33334444u, 0x55556666u, 0x77778888u};
+  const u32 decoy[4] = {0xdeadbeefu, 0xdeadbeefu, 0xdeadbeefu, 0xdeadbeefu};
+  memcpy(mem.at(kA), want, 16);
+  memcpy(mem.at(0), decoy, 16);  // what a dropped offset register would read instead
+  memset(mem.at(kDst), 0, 16);
+
+  run_mem(tester, mem, [](CodeTester& t) {
+    t.emit(IGen::loadvf_gpr64_plus_gpr64(t.generator(), Register(V0), Register(X0), Register(X1)));
+    t.emit(IGen::storevf_gpr64_plus_gpr64(t.generator(), Register(V0), Register(X0), Register(X3)));
+  });
+
+  u32 got[4];
+  memcpy(got, mem.at(kDst), 16);
+  for (int i = 0; i < 4; i++) {
+    EXPECT_EQ(got[i], want[i]) << "reg+reg round trip lane " << i;
+  }
+  tester.clear();
+}
+
+TEST(NEONEmitter, vector_load_store_displaced_reads_base_plus_index_plus_offset) {
+  CodeTester tester = create_tester(2048);
+  VecMem mem;
+
+  // put a marker 16 bytes past kA and another 16 before it, so a wrong displacement is visible
+  const u32 lo[4] = {0xa000u, 0xa001u, 0xa002u, 0xa003u};
+  const u32 mid[4] = {0xb000u, 0xb001u, 0xb002u, 0xb003u};
+  const u32 hi[4] = {0xc000u, 0xc001u, 0xc002u, 0xc003u};
+  memcpy(mem.at(kA - 16), lo, 16);
+  memcpy(mem.at(kA), mid, 16);
+  memcpy(mem.at(kA + 16), hi, 16);
+
+  struct Case {
+    int disp;
+    const u32* want;
+  };
+  const Case cases[] = {{-16, lo}, {0, mid}, {16, hi}};
+
+  for (auto& c : cases) {
+    // s8 form
+    memset(mem.at(kDst), 0, 16);
+    run_mem(tester, mem, [&](CodeTester& t) {
+      t.emit(IGen::loadvf_gpr64_plus_gpr64_plus_s8(t.generator(), Register(V0), Register(X0),
+                                                   Register(X1), c.disp));
+      t.emit(
+          IGen::storevf_gpr64_plus_gpr64(t.generator(), Register(V0), Register(X0), Register(X3)));
+    });
+    u32 got[4];
+    memcpy(got, mem.at(kDst), 16);
+    for (int i = 0; i < 4; i++) {
+      EXPECT_EQ(got[i], c.want[i]) << "load s8 disp " << c.disp << " lane " << i;
+    }
+
+    // s32 form, same answers
+    memset(mem.at(kDst), 0, 16);
+    run_mem(tester, mem, [&](CodeTester& t) {
+      t.emit(IGen::loadvf_gpr64_plus_gpr64_plus_s32(t.generator(), Register(V0), Register(X0),
+                                                    Register(X1), c.disp));
+      t.emit(
+          IGen::storevf_gpr64_plus_gpr64(t.generator(), Register(V0), Register(X0), Register(X3)));
+    });
+    memcpy(got, mem.at(kDst), 16);
+    for (int i = 0; i < 4; i++) {
+      EXPECT_EQ(got[i], c.want[i]) << "load s32 disp " << c.disp << " lane " << i;
+    }
+
+    // and the displaced store lands where it should
+    memset(mem.at(kDst - 16), 0, 48);
+    run_mem(tester, mem, [&](CodeTester& t) {
+      t.emit(
+          IGen::loadvf_gpr64_plus_gpr64(t.generator(), Register(V0), Register(X0), Register(X1)));
+      t.emit(IGen::storevf_gpr64_plus_gpr64_plus_s8(t.generator(), Register(V0), Register(X0),
+                                                    Register(X3), c.disp));
+    });
+    memcpy(got, mem.at(kDst + c.disp), 16);
+    for (int i = 0; i < 4; i++) {
+      EXPECT_EQ(got[i], mid[i]) << "store s8 disp " << c.disp << " lane " << i;
+    }
+  }
+  tester.clear();
+}
+
+TEST(NEONEmitter, displaced_vector_access_leaves_the_address_registers_alone) {
+  CodeTester tester = create_tester(2048);
+  VecMem mem;
+
+  const u32 v[4] = {1, 2, 3, 4};
+  memcpy(mem.at(kA), v, 16);
+
+  // load twice from the same place. a post indexed form would walk the base on the first one
+  // and read somewhere else on the second.
+  memset(mem.at(kDst), 0, 16);
+  run_mem(tester, mem, [](CodeTester& t) {
+    t.emit(IGen::loadvf_gpr64_plus_gpr64_plus_s8(t.generator(), Register(V0), Register(X0),
+                                                 Register(X1), 0));
+    t.emit(IGen::loadvf_gpr64_plus_gpr64_plus_s8(t.generator(), Register(V1), Register(X0),
+                                                 Register(X1), 0));
+    t.emit(IGen::storevf_gpr64_plus_gpr64(t.generator(), Register(V1), Register(X0), Register(X3)));
+  });
+  u32 got[4];
+  memcpy(got, mem.at(kDst), 16);
+  for (int i = 0; i < 4; i++) {
+    EXPECT_EQ(got[i], v[i]) << "second load lane " << i;
+  }
+  tester.clear();
+}
+
+#endif  // __aarch64__