[rocjitsu] Model gfx1250 SRAM ECC semantics for D16 loads

emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py

@@ -3730,10 +3730,14 @@ def _d16_load_reads_dst(self, inst: Instruction) -> bool:
 
         The last register is partial when the written bytes (num_elems *
         elem_size) do not fill whole 32-bit registers (e.g. ushort_d16, or
-        format_d16_xyz).
+        format_d16_xyz). Architectures with SRAM ECC can instead zero-fill the
+        unselected half, making the destination a full write rather than a
+        read-modify-write.
         """
         if not self.semantics:
             return False
+        if self.isa_spec.profile.d16_loads_zero_unselected_half:
+            return False
         sem = self.semantics.instructions.get(inst.name)
         if not sem or sem.num_elems is None or sem.elem_size is None:
             return False
@@ -8196,13 +8200,14 @@ def gen_insts(self) -> None:
                     dst_idx = 0
                     vgpr_msb_src_role_idx = 0
                     reads_dst = self._dst_is_also_source(inst)
-                    # D16(_HI) loads read the destination they partially write.
-                    # Model it as an implicit use so vdst stays out of the
-                    # printed operand list (see _d16_load_reads_dst). The
-                    # override declaration/definition is emitted below, sharing
-                    # the path with generic partial defs (buffer/tbuffer loads
-                    # name the dest 'vdata' and are sized as a full 32-bit
-                    # register, so _partial_def_outputs does not catch them).
+                    # D16(_HI) loads on architectures without SRAM ECC read the
+                    # destination they partially write. Model that preservation
+                    # as an implicit use so vdst stays out of the printed operand
+                    # list (see _d16_load_reads_dst). The override
+                    # declaration/definition is emitted below, sharing the path
+                    # with generic partial defs (buffer/tbuffer loads name the
+                    # dest 'vdata' and are sized as a full 32-bit register, so
+                    # _partial_def_outputs does not catch them).
                     d16_implicit_use_opnd = None
                     # Offset (in 32-bit registers) of the one partially-written
                     # register within the destination: only the last register of

emulation/rocjitsu/lib/python/amdisa/isa_profile.py

@@ -313,6 +313,11 @@ def uses_vgpr_msb_indexing(self) -> bool:
         """True when VGPR operands use MODE-controlled high-bank bits."""
         return False
 
+    @property
+    def d16_loads_zero_unselected_half(self) -> bool:
+        """True when D16 loads zero the unselected destination half."""
+        return False
+
     @property
     def uses_packed_16bit_e32_source_selectors(self) -> bool:
         """True when E32 16-bit source selectors can address packed high halves."""
@@ -1178,6 +1183,11 @@ def uses_true16_vop3_opsel(self) -> bool:
         # zero the upper half; see the CDNA ISA OP_SEL field description.
         return True
 
+    @property
+    def d16_loads_zero_unselected_half(self) -> bool:
+        # CDNA2/3/4 enable SRAM ECC in the runtime ISA traits.
+        return True
+
 
 class Cdna4Profile(CdnaProfile):
     """ISA profile for CDNA4-only encoding capabilities."""
@@ -1218,8 +1228,13 @@ class Cdna1Profile(CdnaProfile):
     - GFX9-style GLC-only coherency model.
     - Scratch base via SGPR pair (not HW register).
     - ENC_VOP3PX2 does not exist in CDNA1 XML.
+    - The current runtime ISA trait leaves SRAM ECC disabled.
     """
 
+    @property
+    def d16_loads_zero_unselected_half(self) -> bool:
+        return False
+
     @property
     def has_acc_vgpr(self) -> bool:
         return False
@@ -1910,6 +1925,10 @@ def vopd3_y_slot_opcodes(self) -> frozenset[int]:
     def uses_vgpr_msb_indexing(self) -> bool:
         return True
 
+    @property
+    def d16_loads_zero_unselected_half(self) -> bool:
+        return True
+
     @property
     def uses_packed_16bit_e32_source_selectors(self) -> bool:
         return True

emulation/rocjitsu/lib/python/amdisa/tests/test_generator_profile_gates.py

@@ -4702,39 +4702,43 @@ def test_only_gfx1250_generates_implicit_use_operands_overrides(
 
 
 @pytest.mark.parametrize(
-    'src_file,class_name',
+    'src_file,class_name,dst_name',
     [
-        ('vflat.cpp', 'FlatLoadD16U8Vflat'),
-        ('vglobal.cpp', 'GlobalLoadD16U8Vglobal'),
-        ('vscratch.cpp', 'ScratchLoadD16U8Vscratch'),
+        ('vbuffer.cpp', 'BufferLoadD16U8Vbuffer', 'vdata'),
+        ('vds.cpp', 'DsLoadU8D16Vds', 'vdst'),
+        ('vflat.cpp', 'FlatLoadD16U8Vflat', 'vdst'),
+        ('vglobal.cpp', 'GlobalLoadD16U8Vglobal', 'vdst'),
+        ('vscratch.cpp', 'ScratchLoadD16U8Vscratch', 'vdst'),
     ],
 )
-def test_gfx1250_d16_load_preserves_destination_without_printing_it(
-    gfx1250_generated_root: Path, src_file, class_name
+def test_gfx1250_d16_load_does_not_preserve_destination(
+    gfx1250_generated_root: Path, src_file, class_name, dst_name
 ):
-    """A D16 partial-def load reads vdst via the implicit hooks, not src_operands_.
+    """A gfx1250 D16 load zero-fills rather than preserve-reading its destination.
 
-    GLOBAL/SCRATCH share semantic_class 'flat_load' with FLAT but emit to
-    separate files, so cover all three. Listing vdst as a source would print it
-    twice in disassembly; pin that a future generator change doing so fails here.
+    Cover every generated memory family because each independently emits its
+    instruction class and hidden-use overrides.
     """
     cpp = (gfx1250_generated_root / src_file).read_text()
     ctor = _generated_constructor_body(cpp, class_name)
 
-    # vdst is a destination, never a source (kept out of the printed operands).
-    assert 'dst_operands_[0] = &vdst;' in ctor
-    assert not re.search(r'src_operands_\[[^\]]*\]\s*=\s*&vdst;', ctor)
+    assert f'dst_operands_[0] = &{dst_name};' in ctor
+    assert not re.search(rf'src_operands_\[[^\]]*\]\s*=\s*&{dst_name};', ctor)
 
-    # The preserved-destination read is modeled via both implicit hooks.
     uses_override = f'void {class_name}::implicit_uses(RegisterSet &uses) const'
     operands_override = f'void {class_name}::implicit_use_operands('
-    assert uses_override in cpp
-    assert operands_override in cpp
-    uses_start = cpp.index(uses_override)
-    uses_body = cpp[uses_start : cpp.index('\n}', uses_start)]
-    assert 'if (auto r = vdst.to_register_ref())' in uses_body
-    assert 'uses.expand(*r);' in uses_body
-    operands_start = cpp.index(operands_override)
-    operands_body = cpp[operands_start : cpp.index('\n}', operands_start)]
-    assert 'if (vdst.to_register_ref())' in operands_body
-    assert 'operands.push_back(&vdst);' in operands_body
+    assert uses_override not in cpp
+    assert operands_override not in cpp
+
+
+def test_cdna4_d16_load_does_not_preserve_destination(
+    amdgpu_generated_root: Path,
+):
+    """CDNA4 D16 loads are full VGPR writes because gfx950 enables SRAM ECC."""
+    cpp = (amdgpu_generated_root / 'cdna4' / 'mubuf.cpp').read_text()
+    class_name = 'BufferLoadUbyteD16Mubuf'
+    ctor = _generated_constructor_body(cpp, class_name)
+
+    assert 'dst_operands_[0] = &vdata;' in ctor
+    assert not re.search(r'src_operands_\[[^\]]*\]\s*=\s*&vdata;', ctor)
+    assert f'void {class_name}::implicit_uses(RegisterSet &uses) const' not in cpp

emulation/rocjitsu/lib/python/amdisa/tests/test_profile_properties.py

@@ -432,6 +432,9 @@ def test_waitcnt_family_default(self):
     def test_waitcnt_lgkmcnt_mask(self):
         assert self.p.waitcnt_lgkmcnt_mask == '0x0F'
 
+    def test_d16_loads_zero_unselected_half(self):
+        assert self.p.d16_loads_zero_unselected_half is True
+
     def test_field_renames_flat(self):
         renames = self.p.field_renames('ENC_FLAT')
         assert renames.get('sve') == 'lds'
@@ -489,6 +492,9 @@ def test_has_mfma_inherited(self):
     def test_wave_size_inherited(self):
         assert self.p.wave_size == 64
 
+    def test_d16_loads_preserve_unselected_half(self):
+        assert self.p.d16_loads_zero_unselected_half is False
+
 
 class TestCdna2Profile:
     """Cdna2Profile: AccVGPR base 512, GFX9_GLC coherency, sgpr_pair scratch."""
@@ -508,6 +514,9 @@ def test_coherency_model(self):
     def test_flat_scratch_mechanism(self):
         assert self.p.flat_scratch_mechanism == 'sgpr_pair'
 
+    def test_d16_loads_zero_unselected_half(self):
+        assert self.p.d16_loads_zero_unselected_half is True
+
 
 class TestRdna1Profile:
     def setup_method(self):
@@ -669,6 +678,9 @@ def test_coherency_model(self):
     def test_vgpr_msb_indexing(self):
         assert self.p.uses_vgpr_msb_indexing is True
 
+    def test_d16_loads_zero_unselected_half(self):
+        assert self.p.d16_loads_zero_unselected_half is True
+
     def test_source_split_limits_leave_precommit_margin(self):
         limits = self.p.source_split_max_bytes
         assert limits['ENC_VOP3'] <= 450 * 1024

emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/cdna5/isa.h

@@ -71,6 +71,7 @@ struct Isa : amdgpu::RdnaIsaBase {
 
   static constexpr uint32_t WF_SIZE_MAX = 32; ///< gfx1250 is Wave32-only.
   static constexpr uint32_t MAX_WF_SLOTS = 64;
+  static constexpr bool SRAM_ECC = true; ///< gfx1250 enables SRAM ECC.
   // gfx1250 uses the RDNA base decoder shape, but its MODE[27] enables VGPR
   // high-bank indexing rather than RDNA4 DISABLE_PERF.
   static constexpr bool MODE_HAS_GPR_IDX_EN = true;

emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/generated/cdna2/ds.cpp

@@ -2674,12 +2674,6 @@ DecodeResult decodeDsReadU8D16Ds(const MachineInst *opcode, const DecodeErrorEmi
 }
 } // namespace detail
 
-void DsReadU8D16Ds::implicit_uses(RegisterSet &uses) const {
-  Ds::implicit_uses(uses);
-  if (auto r = vdst.to_register_ref())
-    uses.expand(*r);
-}
-
 DsReadU8D16HiDs::DsReadU8D16HiDs(const MachineInst *inst)
     : Ds("ds_read_u8_d16_hi", reinterpret_cast<const OpEncoding *>(inst),
          selected_exec_fn(InstructionExecutionId::DsReadU8D16HiDs)),
@@ -2710,12 +2704,6 @@ DecodeResult decodeDsReadU8D16HiDs(const MachineInst *opcode,
 }
 } // namespace detail
 
-void DsReadU8D16HiDs::implicit_uses(RegisterSet &uses) const {
-  Ds::implicit_uses(uses);
-  if (auto r = vdst.to_register_ref())
-    uses.expand(*r);
-}
-
 DsReadI8D16Ds::DsReadI8D16Ds(const MachineInst *inst)
     : Ds("ds_read_i8_d16", reinterpret_cast<const OpEncoding *>(inst),
          selected_exec_fn(InstructionExecutionId::DsReadI8D16Ds)),
@@ -2745,12 +2733,6 @@ DecodeResult decodeDsReadI8D16Ds(const MachineInst *opcode, const DecodeErrorEmi
 }
 } // namespace detail
 
-void DsReadI8D16Ds::implicit_uses(RegisterSet &uses) const {
-  Ds::implicit_uses(uses);
-  if (auto r = vdst.to_register_ref())
-    uses.expand(*r);
-}
-
 DsReadI8D16HiDs::DsReadI8D16HiDs(const MachineInst *inst)
     : Ds("ds_read_i8_d16_hi", reinterpret_cast<const OpEncoding *>(inst),
          selected_exec_fn(InstructionExecutionId::DsReadI8D16HiDs)),
@@ -2781,12 +2763,6 @@ DecodeResult decodeDsReadI8D16HiDs(const MachineInst *opcode,
 }
 } // namespace detail
 
-void DsReadI8D16HiDs::implicit_uses(RegisterSet &uses) const {
-  Ds::implicit_uses(uses);
-  if (auto r = vdst.to_register_ref())
-    uses.expand(*r);
-}
-
 DsReadU16D16Ds::DsReadU16D16Ds(const MachineInst *inst)
     : Ds("ds_read_u16_d16", reinterpret_cast<const OpEncoding *>(inst),
          selected_exec_fn(InstructionExecutionId::DsReadU16D16Ds)),
@@ -2816,12 +2792,6 @@ DecodeResult decodeDsReadU16D16Ds(const MachineInst *opcode, const DecodeErrorEm
 }
 } // namespace detail
 
-void DsReadU16D16Ds::implicit_uses(RegisterSet &uses) const {
-  Ds::implicit_uses(uses);
-  if (auto r = vdst.to_register_ref())
-    uses.expand(*r);
-}
-
 DsReadU16D16HiDs::DsReadU16D16HiDs(const MachineInst *inst)
     : Ds("ds_read_u16_d16_hi", reinterpret_cast<const OpEncoding *>(inst),
          selected_exec_fn(InstructionExecutionId::DsReadU16D16HiDs)),
@@ -2852,12 +2822,6 @@ DecodeResult decodeDsReadU16D16HiDs(const MachineInst *opcode,
 }
 } // namespace detail
 
-void DsReadU16D16HiDs::implicit_uses(RegisterSet &uses) const {
-  Ds::implicit_uses(uses);
-  if (auto r = vdst.to_register_ref())
-    uses.expand(*r);
-}
-
 DsAddF64Ds::DsAddF64Ds(const MachineInst *inst)
     : Ds("ds_add_f64", reinterpret_cast<const OpEncoding *>(inst),
          selected_exec_fn(InstructionExecutionId::DsAddF64Ds)),

emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/generated/cdna2/ds.h

@@ -820,7 +820,6 @@ class DsReadU8D16Ds : public Ds {
 public:
   DsReadU8D16Ds(const MachineInst *inst);
   void execute_impl(amdgpu::Wavefront &wf);
-  void implicit_uses(RegisterSet &uses) const override;
   Operand vdst;
   Operand addr;
   Operand dsmem;
@@ -830,7 +829,6 @@ class DsReadU8D16HiDs : public Ds {
 public:
   DsReadU8D16HiDs(const MachineInst *inst);
   void execute_impl(amdgpu::Wavefront &wf);
-  void implicit_uses(RegisterSet &uses) const override;
   Operand vdst;
   Operand addr;
   Operand dsmem;
@@ -840,7 +838,6 @@ class DsReadI8D16Ds : public Ds {
 public:
   DsReadI8D16Ds(const MachineInst *inst);
   void execute_impl(amdgpu::Wavefront &wf);
-  void implicit_uses(RegisterSet &uses) const override;
   Operand vdst;
   Operand addr;
   Operand dsmem;
@@ -850,7 +847,6 @@ class DsReadI8D16HiDs : public Ds {
 public:
   DsReadI8D16HiDs(const MachineInst *inst);
   void execute_impl(amdgpu::Wavefront &wf);
-  void implicit_uses(RegisterSet &uses) const override;
   Operand vdst;
   Operand addr;
   Operand dsmem;
@@ -860,7 +856,6 @@ class DsReadU16D16Ds : public Ds {
 public:
   DsReadU16D16Ds(const MachineInst *inst);
   void execute_impl(amdgpu::Wavefront &wf);
-  void implicit_uses(RegisterSet &uses) const override;
   Operand vdst;
   Operand addr;
   Operand dsmem;
@@ -870,7 +865,6 @@ class DsReadU16D16HiDs : public Ds {
 public:
   DsReadU16D16HiDs(const MachineInst *inst);
   void execute_impl(amdgpu::Wavefront &wf);
-  void implicit_uses(RegisterSet &uses) const override;
   Operand vdst;
   Operand addr;
   Operand dsmem;