aco: validate instructions reading/writing upper halves/bytes

Signed-off-by: Rhys Perry <pendingchaos02@gmail.com>
Reviewed-by: Daniel Schürmann <daniel@schuermann.dev>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5040>

src/amd/compiler/aco_ir.cpp

@@ -0,0 +1,74 @@
+/*
+ * Copyright © 2020 Valve Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+#include "aco_ir.h"
+
+namespace aco {
+
+bool can_use_opsel(chip_class chip, aco_opcode op, int idx, bool high)
+{
+   /* opsel is only GFX9+ */
+   if ((high || idx == -1) && chip < GFX9)
+      return false;
+
+   switch (op) {
+   case aco_opcode::v_div_fixup_f16:
+   case aco_opcode::v_fma_f16:
+   case aco_opcode::v_mad_f16:
+   case aco_opcode::v_mad_u16:
+   case aco_opcode::v_mad_i16:
+   case aco_opcode::v_med3_f16:
+   case aco_opcode::v_med3_i16:
+   case aco_opcode::v_med3_u16:
+   case aco_opcode::v_min3_f16:
+   case aco_opcode::v_min3_i16:
+   case aco_opcode::v_min3_u16:
+   case aco_opcode::v_max3_f16:
+   case aco_opcode::v_max3_i16:
+   case aco_opcode::v_max3_u16:
+   case aco_opcode::v_max_u16_e64:
+   case aco_opcode::v_max_i16_e64:
+   case aco_opcode::v_min_u16_e64:
+   case aco_opcode::v_min_i16_e64:
+   case aco_opcode::v_add_i16:
+   case aco_opcode::v_sub_i16:
+   case aco_opcode::v_add_u16_e64:
+   case aco_opcode::v_sub_u16_e64:
+   case aco_opcode::v_cvt_pknorm_i16_f16:
+   case aco_opcode::v_cvt_pknorm_u16_f16:
+   case aco_opcode::v_lshlrev_b16_e64:
+   case aco_opcode::v_lshrrev_b16_e64:
+   case aco_opcode::v_ashrrev_i16_e64:
+   case aco_opcode::v_mul_lo_u16_e64:
+      return true;
+   case aco_opcode::v_pack_b32_f16:
+      return idx != -1;
+   case aco_opcode::v_mad_u32_u16:
+   case aco_opcode::v_mad_i32_i16:
+      return idx >= 0 && idx < 2;
+   default:
+      return false;
+   }
+}
+
+}

src/amd/compiler/aco_ir.h

@@ -1223,6 +1223,8 @@ barrier_interaction get_barrier_interaction(const Instruction* instr);
 
 bool is_dead(const std::vector<uint16_t>& uses, Instruction *instr);
 
+bool can_use_opsel(chip_class chip, aco_opcode op, int idx, bool high);
+
 enum block_kind {
    /* uniform indicates that leaving this block,
     * all actives lanes stay active */
@@ -1430,6 +1432,7 @@ public:
    unsigned workgroup_size; /* if known; otherwise UINT_MAX */
 
    bool xnack_enabled = false;
+   bool sram_ecc_enabled = false;
 
    bool needs_vcc = false;
    bool needs_flat_scr = false;

src/amd/compiler/aco_validate.cpp

@@ -146,6 +146,13 @@ void validate(Program* program, FILE * output)
                      instr->opcode != aco_opcode::v_fmac_f16,
                      "SDWA can't be used with this opcode", instr.get());
             }
+
+            for (unsigned i = 0; i < MIN2(instr->operands.size(), 2); i++) {
+               if (instr->operands[i].regClass().is_subdword())
+                  check((sdwa->sel[i] & sdwa_asuint) == (sdwa_isra | instr->operands[i].bytes()), "Unexpected SDWA sel for sub-dword operand", instr.get());
+            }
+            if (instr->definitions[0].regClass().is_subdword())
+               check((sdwa->dst_sel & sdwa_asuint) == (sdwa_isra | instr->definitions[0].bytes()), "Unexpected SDWA sel for sub-dword definition", instr.get());
          }
 
          /* check opsel */
@@ -153,6 +160,13 @@ void validate(Program* program, FILE * output)
             VOP3A_instruction *vop3 = static_cast<VOP3A_instruction*>(instr.get());
             check(vop3->opsel == 0 || program->chip_class >= GFX9, "Opsel is only supported on GFX9+", instr.get());
             check((vop3->opsel & ~(0x10 | ((1 << instr->operands.size()) - 1))) == 0, "Unused bits in opsel must be zeroed out", instr.get());
+
+            for (unsigned i = 0; i < instr->operands.size(); i++) {
+               if (instr->operands[i].regClass().is_subdword())
+                  check((vop3->opsel & (1 << i)) == 0, "Unexpected opsel for sub-dword operand", instr.get());
+            }
+            if (instr->definitions[0].regClass().is_subdword())
+               check((vop3->opsel & (1 << 3)) == 0, "Unexpected opsel for sub-dword definition", instr.get());
          }
 
          /* check for undefs */
@@ -429,11 +443,132 @@ bool ra_fail(FILE *output, Location loc, Location loc2, const char *fmt, ...) {
    return true;
 }
 
-bool instr_can_access_subdword(Program* program, aco_ptr<Instruction>& instr)
+bool validate_subdword_operand(chip_class chip, const aco_ptr<Instruction>& instr, unsigned index)
 {
-   if (program->chip_class < GFX8)
-      return false;
-   return instr->isSDWA() || instr->format == Format::PSEUDO;
+   Operand op = instr->operands[index];
+   unsigned byte = op.physReg().byte();
+
+   if (instr->format == Format::PSEUDO && chip >= GFX8)
+      return true;
+   if (instr->isSDWA() && (static_cast<SDWA_instruction *>(instr.get())->sel[index] & sdwa_asuint) == (sdwa_isra | op.bytes()))
+      return true;
+   if (byte == 2 && can_use_opsel(chip, instr->opcode, index, 1))
+      return true;
+
+   switch (instr->opcode) {
+   case aco_opcode::v_cvt_f32_ubyte1:
+      if (byte == 1)
+         return true;
+      break;
+   case aco_opcode::v_cvt_f32_ubyte2:
+      if (byte == 2)
+         return true;
+      break;
+   case aco_opcode::v_cvt_f32_ubyte3:
+      if (byte == 3)
+         return true;
+      break;
+   case aco_opcode::ds_write_b8_d16_hi:
+   case aco_opcode::ds_write_b16_d16_hi:
+      if (byte == 2 && index == 1)
+         return true;
+      break;
+   case aco_opcode::buffer_store_byte_d16_hi:
+   case aco_opcode::buffer_store_short_d16_hi:
+      if (byte == 2 && index == 3)
+         return true;
+      break;
+   case aco_opcode::flat_store_byte_d16_hi:
+   case aco_opcode::flat_store_short_d16_hi:
+   case aco_opcode::scratch_store_byte_d16_hi:
+   case aco_opcode::scratch_store_short_d16_hi:
+   case aco_opcode::global_store_byte_d16_hi:
+   case aco_opcode::global_store_short_d16_hi:
+      if (byte == 2 && index == 2)
+         return true;
+   default:
+      break;
+   }
+
+   return byte == 0;
+}
+
+bool validate_subdword_definition(chip_class chip, const aco_ptr<Instruction>& instr)
+{
+   Definition def = instr->definitions[0];
+   unsigned byte = def.physReg().byte();
+
+   if (instr->format == Format::PSEUDO && chip >= GFX8)
+      return true;
+   if (instr->isSDWA() && static_cast<SDWA_instruction *>(instr.get())->dst_sel == (sdwa_isra | def.bytes()))
+      return true;
+   if (byte == 2 && can_use_opsel(chip, instr->opcode, -1, 1))
+      return true;
+
+   switch (instr->opcode) {
+   case aco_opcode::buffer_load_ubyte_d16_hi:
+   case aco_opcode::buffer_load_short_d16_hi:
+   case aco_opcode::flat_load_ubyte_d16_hi:
+   case aco_opcode::flat_load_short_d16_hi:
+   case aco_opcode::scratch_load_ubyte_d16_hi:
+   case aco_opcode::scratch_load_short_d16_hi:
+   case aco_opcode::global_load_ubyte_d16_hi:
+   case aco_opcode::global_load_short_d16_hi:
+   case aco_opcode::ds_read_u8_d16_hi:
+   case aco_opcode::ds_read_u16_d16_hi:
+      return byte == 2;
+   default:
+      break;
+   }
+
+   return byte == 0;
+}
+
+unsigned get_subdword_bytes_written(Program *program, const aco_ptr<Instruction>& instr, unsigned index)
+{
+   chip_class chip = program->chip_class;
+   Definition def = instr->definitions[index];
+
+   if (instr->format == Format::PSEUDO)
+      return chip >= GFX8 ? def.bytes() : def.size() * 4u;
+   if (instr->isSDWA() && static_cast<SDWA_instruction *>(instr.get())->dst_sel == (sdwa_isra | def.bytes()))
+      return def.bytes();
+
+   switch (instr->opcode) {
+   case aco_opcode::buffer_load_ubyte_d16:
+   case aco_opcode::buffer_load_short_d16:
+   case aco_opcode::flat_load_ubyte_d16:
+   case aco_opcode::flat_load_short_d16:
+   case aco_opcode::scratch_load_ubyte_d16:
+   case aco_opcode::scratch_load_short_d16:
+   case aco_opcode::global_load_ubyte_d16:
+   case aco_opcode::global_load_short_d16:
+   case aco_opcode::ds_read_u8_d16:
+   case aco_opcode::ds_read_u16_d16:
+   case aco_opcode::buffer_load_ubyte_d16_hi:
+   case aco_opcode::buffer_load_short_d16_hi:
+   case aco_opcode::flat_load_ubyte_d16_hi:
+   case aco_opcode::flat_load_short_d16_hi:
+   case aco_opcode::scratch_load_ubyte_d16_hi:
+   case aco_opcode::scratch_load_short_d16_hi:
+   case aco_opcode::global_load_ubyte_d16_hi:
+   case aco_opcode::global_load_short_d16_hi:
+   case aco_opcode::ds_read_u8_d16_hi:
+   case aco_opcode::ds_read_u16_d16_hi:
+      return program->sram_ecc_enabled ? 4 : 2;
+   case aco_opcode::v_mad_f16:
+   case aco_opcode::v_mad_u16:
+   case aco_opcode::v_mad_i16:
+   case aco_opcode::v_fma_f16:
+   case aco_opcode::v_div_fixup_f16:
+   case aco_opcode::v_interp_p2_f16:
+      if (chip >= GFX9)
+         return 2;
+   default:
+      break;
+   }
+
+   return chip >= GFX10 ? def.bytes() : 4;
 }
 
 } /* end namespace */
@@ -474,8 +609,8 @@ bool validate_ra(Program *program, const struct radv_nir_compiler_options *optio
                err |= ra_fail(output, loc, assignments.at(op.tempId()).firstloc, "Operand %d has an out-of-bounds register assignment", i);
             if (op.physReg() == vcc && !program->needs_vcc)
                err |= ra_fail(output, loc, Location(), "Operand %d fixed to vcc but needs_vcc=false", i);
-            if (!instr_can_access_subdword(program, instr) && op.regClass().is_subdword() && op.physReg().byte())
-               err |= ra_fail(output, loc, assignments.at(op.tempId()).firstloc, "Operand %d must be aligned to a full register", i);
+            if (op.regClass().is_subdword() && !validate_subdword_operand(program->chip_class, instr, i))
+               err |= ra_fail(output, loc, Location(), "Operand %d not aligned correctly", i);
             if (!assignments[op.tempId()].firstloc.block)
                assignments[op.tempId()].firstloc = loc;
             if (!assignments[op.tempId()].defloc.block)
@@ -495,8 +630,8 @@ bool validate_ra(Program *program, const struct radv_nir_compiler_options *optio
                err |= ra_fail(output, loc, assignments.at(def.tempId()).firstloc, "Definition %d has an out-of-bounds register assignment", i);
             if (def.physReg() == vcc && !program->needs_vcc)
                err |= ra_fail(output, loc, Location(), "Definition %d fixed to vcc but needs_vcc=false", i);
-            if (!instr_can_access_subdword(program, instr) && def.regClass().is_subdword() && def.physReg().byte())
-               err |= ra_fail(output, loc, assignments.at(def.tempId()).firstloc, "Definition %d must be aligned to a full register", i);
+            if (def.regClass().is_subdword() && !validate_subdword_definition(program->chip_class, instr))
+               err |= ra_fail(output, loc, Location(), "Definition %d not aligned correctly", i);
             if (!assignments[def.tempId()].firstloc.block)
                assignments[def.tempId()].firstloc = loc;
             assignments[def.tempId()].defloc = loc;
@@ -602,10 +737,14 @@ bool validate_ra(Program *program, const struct radv_nir_compiler_options *optio
                   err |= ra_fail(output, loc, assignments.at(regs[reg.reg_b + j]).defloc, "Assignment of element %d of %%%d already taken by %%%d from instruction", i, tmp.id(), regs[reg.reg_b + j]);
                regs[reg.reg_b + j] = tmp.id();
             }
-            if (def.regClass().is_subdword() && !instr_can_access_subdword(program, instr)) {
-               for (unsigned j = tmp.bytes(); j < 4; j++)
-                  if (regs[reg.reg_b + j])
-                     err |= ra_fail(output, loc, assignments.at(regs[reg.reg_b + j]).defloc, "Assignment of element %d of %%%d overwrites the full register taken by %%%d from instruction", i, tmp.id(), regs[reg.reg_b + j]);
+            if (def.regClass().is_subdword() && def.bytes() < 4) {
+               unsigned written = get_subdword_bytes_written(program, instr, i);
+               /* If written=4, the instruction still might write the upper half. In that case, it's the lower half that isn't preserved */
+               for (unsigned j = reg.byte() & ~(written - 1); j < written; j++) {
+                  unsigned written_reg = reg.reg() * 4u + j;
+                  if (regs[written_reg] && regs[written_reg] != def.tempId())
+                     err |= ra_fail(output, loc, assignments.at(regs[written_reg]).defloc, "Assignment of element %d of %%%d overwrites the full register taken by %%%d from instruction", i, tmp.id(), regs[written_reg]);
+               }
             }
          }
 

src/amd/compiler/meson.build

@@ -60,6 +60,7 @@ libaco_files = files(
   'aco_instruction_selection_setup.cpp',
   'aco_interface.cpp',
   'aco_interface.h',
+  'aco_ir.cpp',
   'aco_ir.h',
   'aco_assembler.cpp',
   'aco_insert_exec_mask.cpp',