brw: Don't mark_invalid in update_for_reads for non-VGRF destination

This can occur if NULL or an accumulator is an explicit destination.
update_for_reads still needs to process the sources.

v2: Pass a brw_reg to ::mark_invalid, and do the VGRF check in that one
place.

Fixes: 0d144821f0 ("intel/brw: Add a new def analysis pass")
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
(cherry picked from commit a548466186)

Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/40359>
This commit is contained in:
Ian Romanick 2026-02-12 17:07:45 -08:00 committed by Eric Engestrom
parent 31ea1923de
commit f21bc439a1
5 changed files with 82 additions and 11 deletions

View file

@ -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

View file

@ -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 *);

View file

@ -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 &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;
}

View file

@ -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',

View file

@ -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));
}