From 66dc6e07f5c8124dca5122eb74bf98b50ba0a9fb Mon Sep 17 00:00:00 2001 From: Ian Romanick Date: Wed, 9 Aug 2023 14:03:57 -0700 Subject: [PATCH] intel/brw: Fix handling of accumulator register numbers Folks, there's more than one accumulator. In general, when the register file is ARF, the upper 4 bits of the register number specify which ARF, and the lower 4 bits specify which one of that ARF. This can be further partitioned by the subregister number. This is already mostly handled correctly for flags register, but lots of places wanted to check the register number for equality with BRW_ARF_ACCUMULATOR. If acc1 is ever specified, that won't work. Reviewed-by: Lionel Landwerlin Reviewed-by: Caio Oliveira Part-of: --- src/intel/compiler/brw_eu_emit.c | 29 ++++++++++++---------------- src/intel/compiler/brw_eu_validate.c | 2 +- src/intel/compiler/brw_fs.cpp | 16 +++++++++++---- src/intel/compiler/brw_shader.cpp | 2 +- 4 files changed, 26 insertions(+), 23 deletions(-) diff --git a/src/intel/compiler/brw_eu_emit.c b/src/intel/compiler/brw_eu_emit.c index c254fbb412f..fa94cc831c7 100644 --- a/src/intel/compiler/brw_eu_emit.c +++ b/src/intel/compiler/brw_eu_emit.c @@ -276,7 +276,7 @@ brw_set_src1(struct brw_codegen *p, brw_inst *inst, struct brw_reg reg) * operands only." */ assert(reg.file != BRW_ARCHITECTURE_REGISTER_FILE || - reg.nr != BRW_ARF_ACCUMULATOR); + (reg.nr & 0xF0) != BRW_ARF_ACCUMULATOR); brw_inst_set_src1_file_type(devinfo, inst, reg.file, reg.type); brw_inst_set_src1_abs(devinfo, inst, reg.abs); @@ -600,24 +600,19 @@ brw_alu3(struct brw_codegen *p, unsigned opcode, struct brw_reg dest, if (brw_inst_access_mode(devinfo, inst) == BRW_ALIGN_1) { assert(dest.file == BRW_GENERAL_REGISTER_FILE || (dest.file == BRW_ARCHITECTURE_REGISTER_FILE && - dest.nr == BRW_ARF_ACCUMULATOR)); + (dest.nr & 0xF0) == BRW_ARF_ACCUMULATOR)); - if (devinfo->ver >= 12) { - brw_inst_set_3src_a1_dst_reg_file(devinfo, inst, dest.file); - brw_inst_set_3src_dst_reg_nr(devinfo, inst, phys_nr(devinfo, dest)); - } else { - if (dest.file == BRW_ARCHITECTURE_REGISTER_FILE) { - brw_inst_set_3src_a1_dst_reg_file(devinfo, inst, - BRW_ALIGN1_3SRC_ACCUMULATOR); - brw_inst_set_3src_dst_reg_nr(devinfo, inst, BRW_ARF_ACCUMULATOR); - } else { - brw_inst_set_3src_a1_dst_reg_file(devinfo, inst, - BRW_ALIGN1_3SRC_GENERAL_REGISTER_FILE); - brw_inst_set_3src_dst_reg_nr(devinfo, inst, dest.nr); - } - } + STATIC_ASSERT((BRW_ARCHITECTURE_REGISTER_FILE ^ 1) == BRW_ALIGN1_3SRC_ACCUMULATOR); + STATIC_ASSERT((BRW_GENERAL_REGISTER_FILE ^ 1) == BRW_ALIGN1_3SRC_GENERAL_REGISTER_FILE); + + /* Gfx10 and Gfx11 bit encoding for the register file is the inversion of + * the actual register file (see the STATIC_ASSERTs above). + */ + unsigned dst_reg_file = devinfo->ver >= 12 ? dest.file : dest.file ^ 1; + + brw_inst_set_3src_a1_dst_reg_file(devinfo, inst, dst_reg_file); + brw_inst_set_3src_dst_reg_nr(devinfo, inst, phys_nr(devinfo, dest)); brw_inst_set_3src_a1_dst_subreg_nr(devinfo, inst, phys_subnr(devinfo, dest) / 8); - brw_inst_set_3src_a1_dst_hstride(devinfo, inst, BRW_ALIGN1_3SRC_DST_HORIZONTAL_STRIDE_1); if (brw_reg_type_is_floating_point(dest.type)) { diff --git a/src/intel/compiler/brw_eu_validate.c b/src/intel/compiler/brw_eu_validate.c index 0357eaaea12..ff235673b7d 100644 --- a/src/intel/compiler/brw_eu_validate.c +++ b/src/intel/compiler/brw_eu_validate.c @@ -1732,7 +1732,7 @@ special_requirements_for_handling_double_precision_data_types( ERROR_IF((address_mode == BRW_ADDRESS_DIRECT && file == BRW_ARCHITECTURE_REGISTER_FILE && reg != BRW_ARF_NULL && !(reg >= BRW_ARF_ACCUMULATOR && reg < BRW_ARF_FLAG)) || (dst_file == BRW_ARCHITECTURE_REGISTER_FILE && - dst_reg != BRW_ARF_NULL && dst_reg != BRW_ARF_ACCUMULATOR), + dst_reg != BRW_ARF_NULL && (dst_reg & 0xF0) != BRW_ARF_ACCUMULATOR), "Explicit ARF registers except null and accumulator must not " "be used."); } diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp index ca1bcf4fd74..b970312f40c 100644 --- a/src/intel/compiler/brw_fs.cpp +++ b/src/intel/compiler/brw_fs.cpp @@ -2538,7 +2538,7 @@ fs_visitor::dump_instruction_to_file(const fs_inst *inst, FILE *file) const fprintf(file, "***attr%d***", inst->dst.nr); break; case ARF: - switch (inst->dst.nr) { + switch (inst->dst.nr & 0xF0) { case BRW_ARF_NULL: fprintf(file, "null"); break; @@ -2546,7 +2546,11 @@ fs_visitor::dump_instruction_to_file(const fs_inst *inst, FILE *file) const fprintf(file, "a0.%d", inst->dst.subnr); break; case BRW_ARF_ACCUMULATOR: - fprintf(file, "acc%d", inst->dst.subnr); + if (inst->dst.subnr == 0) + fprintf(file, "acc%d", inst->dst.nr & 0x0F); + else + fprintf(file, "acc%d.%d", inst->dst.nr & 0x0F, inst->dst.subnr); + break; case BRW_ARF_FLAG: fprintf(file, "f%d.%d", inst->dst.nr & 0xf, inst->dst.subnr); @@ -2636,7 +2640,7 @@ fs_visitor::dump_instruction_to_file(const fs_inst *inst, FILE *file) const } break; case ARF: - switch (inst->src[i].nr) { + switch (inst->src[i].nr & 0xF0) { case BRW_ARF_NULL: fprintf(file, "null"); break; @@ -2644,7 +2648,11 @@ fs_visitor::dump_instruction_to_file(const fs_inst *inst, FILE *file) const fprintf(file, "a0.%d", inst->src[i].subnr); break; case BRW_ARF_ACCUMULATOR: - fprintf(file, "acc%d", inst->src[i].subnr); + if (inst->src[i].subnr == 0) + fprintf(file, "acc%d", inst->src[i].nr & 0x0F); + else + fprintf(file, "acc%d.%d", inst->src[i].nr & 0x0F, inst->src[i].subnr); + break; case BRW_ARF_FLAG: fprintf(file, "f%d.%d", inst->src[i].nr & 0xf, inst->src[i].subnr); diff --git a/src/intel/compiler/brw_shader.cpp b/src/intel/compiler/brw_shader.cpp index c16c9a77146..241063414ab 100644 --- a/src/intel/compiler/brw_shader.cpp +++ b/src/intel/compiler/brw_shader.cpp @@ -282,7 +282,7 @@ fs_reg::is_null() const bool fs_reg::is_accumulator() const { - return file == ARF && nr == BRW_ARF_ACCUMULATOR; + return file == ARF && (nr & 0xF0) == BRW_ARF_ACCUMULATOR; } bool