diff --git a/.pick_status.json b/.pick_status.json index a0c52bbb6fe..6c70329ec72 100644 --- a/.pick_status.json +++ b/.pick_status.json @@ -2964,7 +2964,7 @@ "description": "brw: Don't mark_invalid in update_for_reads for non-VGRF destination", "nominated": true, "nomination_type": 2, - "resolution": 0, + "resolution": 1, "main_sha": null, "because_sha": "0d144821f0f7647ab377e629599b17dd24a2fe13", "notes": null diff --git a/src/intel/compiler/brw/brw_analysis.h b/src/intel/compiler/brw/brw_analysis.h index ed8ec1a70fa..afb9cc0833f 100644 --- a/src/intel/compiler/brw/brw_analysis.h +++ b/src/intel/compiler/brw/brw_analysis.h @@ -396,7 +396,7 @@ public: bool validate(const brw_shader *) const; private: - void mark_invalid(int); + void mark_invalid(const brw_reg &); bool fully_defines(const brw_shader *v, brw_inst *); void update_for_reads(const brw_idom_tree &idom, brw_inst *); void update_for_write(const brw_shader *v, brw_inst *); diff --git a/src/intel/compiler/brw/brw_analysis_def.cpp b/src/intel/compiler/brw/brw_analysis_def.cpp index f813a45413b..ae94219e9a9 100644 --- a/src/intel/compiler/brw/brw_analysis_def.cpp +++ b/src/intel/compiler/brw/brw_analysis_def.cpp @@ -37,9 +37,10 @@ static brw_inst *const UNSEEN = (brw_inst *) (uintptr_t) 1; void -brw_def_analysis::mark_invalid(int nr) +brw_def_analysis::mark_invalid(const brw_reg ®) { - def_insts[nr] = NULL; + if (reg.file == VGRF) + def_insts[reg.nr] = NULL; } void @@ -50,7 +51,7 @@ brw_def_analysis::update_for_reads(const brw_idom_tree &idom, * implicitly reads the accumulator, we don't consider it to produce a def. */ if (inst->reads_accumulator_implicitly()) - mark_invalid(inst->dst.nr); + mark_invalid(inst->dst); for (int i = 0; i < inst->sources; i++) { const int nr = inst->src[i].nr; @@ -63,7 +64,7 @@ brw_def_analysis::update_for_reads(const brw_idom_tree &idom, (nr == BRW_ARF_ADDRESS || nr == BRW_ARF_ACCUMULATOR || nr == BRW_ARF_FLAG)) - mark_invalid(inst->dst.nr); + mark_invalid(inst->dst); continue; } @@ -79,7 +80,7 @@ brw_def_analysis::update_for_reads(const brw_idom_tree &idom, */ if (def_insts[nr] == UNSEEN || !idom.dominates(def_insts[nr]->block, inst->block)) - mark_invalid(nr); + mark_invalid(inst->src[i]); } /* Additionally, if one of our sources is not a def, then our @@ -87,8 +88,8 @@ brw_def_analysis::update_for_reads(const brw_idom_tree &idom, */ if (inst->opcode != SHADER_OPCODE_LOAD_REG && inst->opcode != SHADER_OPCODE_LOAD_PAYLOAD && - !def_insts[nr] && inst->dst.file == VGRF) - mark_invalid(inst->dst.nr); + !def_insts[nr]) + mark_invalid(inst->dst); } } @@ -117,7 +118,7 @@ brw_def_analysis::update_for_write(const brw_shader *v, /* Otherwise this is a second write or a partial write, in which * case we know with certainty that this isn't an SSA def. */ - mark_invalid(nr); + mark_invalid(inst->dst); } } @@ -162,7 +163,7 @@ brw_def_analysis::brw_def_analysis(const brw_shader *v) /* If our "def" reads a non-SSA source, then it isn't a def. */ if (def->opcode != SHADER_OPCODE_LOAD_REG && (!def_insts[nr] || def_insts[nr] == UNSEEN)) { - mark_invalid(def->dst.nr); + mark_invalid(def->dst); iterate = true; break; } diff --git a/src/intel/compiler/brw/meson.build b/src/intel/compiler/brw/meson.build index 8e80579072d..ccef45181c4 100644 --- a/src/intel/compiler/brw/meson.build +++ b/src/intel/compiler/brw/meson.build @@ -159,6 +159,7 @@ if with_tests executable( 'intel_compiler_brw_tests', files( + 'test_def_analysis.cpp', 'test_eu_compact.cpp', 'test_eu_validate.cpp', 'test_helpers.cpp', diff --git a/src/intel/compiler/brw/test_def_analysis.cpp b/src/intel/compiler/brw/test_def_analysis.cpp new file mode 100644 index 00000000000..f2da188543c --- /dev/null +++ b/src/intel/compiler/brw/test_def_analysis.cpp @@ -0,0 +1,69 @@ +/* + * Copyright 2026 Intel Corporation + * SPDX-License-Identifier: MIT + */ + +#include "test_helpers.h" +#include "brw_builder.h" + +class defs_test : public brw_shader_pass_test {}; + +TEST_F(defs_test, dst_is_acc0) +{ + set_gfx_verx10(125); + + brw_builder bld = make_shader(MESA_SHADER_FRAGMENT, 16); + brw_reg acc0 = retype(brw_acc_reg(16), BRW_TYPE_F); + brw_reg dst0; + + for (unsigned i = 0; i < 1024; i++) { + dst0 = vgrf(bld, BRW_TYPE_F); + + if (dst0.nr == acc0.nr) + break; + } + + ASSERT_EQ(dst0.nr, acc0.nr); + + brw_reg src0 = vgrf(bld, BRW_TYPE_F); + + brw_inst *inst = bld.MOV(dst0, brw_imm_f(1.0)); + bld.MOV(src0, brw_imm_f(2.0)); + bld.MAC(acc0, src0, brw_imm_f(3.0)); + + brw_calculate_cfg(*bld.shader); + brw_validate(*bld.shader); + + const brw_def_analysis &defs = bld.shader->def_analysis.require(); + + EXPECT_EQ(inst, defs.get(dst0)); +} + +TEST_F(defs_test, dst_and_src_are_acc0) +{ + set_gfx_verx10(125); + + brw_builder bld = make_shader(MESA_SHADER_FRAGMENT, 16); + brw_reg acc0 = retype(brw_acc_reg(16), BRW_TYPE_F); + brw_reg dst0; + + for (unsigned i = 0; i < 1024; i++) { + dst0 = vgrf(bld, BRW_TYPE_F); + + if (dst0.nr == acc0.nr) + break; + } + + ASSERT_EQ(dst0.nr, acc0.nr); + + brw_inst *inst = bld.MOV(dst0, brw_imm_f(1.0)); + bld.MOV(acc0, brw_imm_f(2.0)); + bld.MUL(acc0, acc0, brw_imm_f(3.0)); + + brw_calculate_cfg(*bld.shader); + brw_validate(*bld.shader); + + const brw_def_analysis &defs = bld.shader->def_analysis.require(); + + EXPECT_EQ(inst, defs.get(dst0)); +}