nir: Avoid visiting instructions multiple times in nir_instr_free_and_dce.

Sadly need to poke a bit in the src internals to avoid using yet another
heap allocated datastructure.

Fixes: 5251548572 ("nir: Add a nir_instr_remove that recursively removes dead code.")
Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/5323
Reviewed-by: Emma Anholt <emma@anholt.net>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12726>
This commit is contained in:
Bas Nieuwenhuizen 2021-09-04 14:27:58 +02:00 committed by Marge Bot
parent ca1622830e
commit b05cd10b8e
2 changed files with 54 additions and 14 deletions

View file

@ -1120,6 +1120,29 @@ static bool nir_instr_free_and_dce_is_live(nir_instr *instr)
return live;
}
static bool
nir_instr_dce_add_dead_srcs_cb(nir_src *src, void *state)
{
nir_instr_worklist *wl = state;
if (src->is_ssa) {
list_del(&src->use_link);
if (!nir_instr_free_and_dce_is_live(src->ssa->parent_instr))
nir_instr_worklist_push_tail(wl, src->ssa->parent_instr);
/* Stop nir_instr_remove from trying to delete the link again. */
src->ssa = NULL;
}
return true;
}
static void
nir_instr_dce_add_dead_ssa_srcs(nir_instr_worklist *wl, nir_instr *instr)
{
nir_foreach_src(instr, nir_instr_dce_add_dead_srcs_cb, wl);
}
/**
* Frees an instruction and any SSA defs that it used that are now dead,
* returning a nir_cursor where the instruction previously was.
@ -1129,7 +1152,7 @@ nir_instr_free_and_dce(nir_instr *instr)
{
nir_instr_worklist *worklist = nir_instr_worklist_create();
nir_instr_worklist_add_ssa_srcs(worklist, instr);
nir_instr_dce_add_dead_ssa_srcs(worklist, instr);
nir_cursor c = nir_instr_remove(instr);
struct exec_list to_free;
@ -1137,20 +1160,18 @@ nir_instr_free_and_dce(nir_instr *instr)
nir_instr *dce_instr;
while ((dce_instr = nir_instr_worklist_pop_head(worklist))) {
if (!nir_instr_free_and_dce_is_live(dce_instr)) {
nir_instr_worklist_add_ssa_srcs(worklist, dce_instr);
nir_instr_dce_add_dead_ssa_srcs(worklist, dce_instr);
/* If we're removing the instr where our cursor is, then we have to
* point the cursor elsewhere.
*/
if ((c.option == nir_cursor_before_instr ||
c.option == nir_cursor_after_instr) &&
c.instr == dce_instr)
c = nir_instr_remove(dce_instr);
else
nir_instr_remove(dce_instr);
exec_list_push_tail(&to_free, &dce_instr->node);
}
/* If we're removing the instr where our cursor is, then we have to
* point the cursor elsewhere.
*/
if ((c.option == nir_cursor_before_instr ||
c.option == nir_cursor_after_instr) &&
c.instr == dce_instr)
c = nir_instr_remove(dce_instr);
else
nir_instr_remove(dce_instr);
exec_list_push_tail(&to_free, &dce_instr->node);
}
struct exec_node *node;

View file

@ -123,4 +123,23 @@ TEST_F(nir_core_test, nir_instr_free_and_dce_all_test)
nir_validate_shader(b->shader, "after remove_and_dce");
}
TEST_F(nir_core_test, nir_instr_free_and_dce_multiple_src_test)
{
nir_ssa_def *one = nir_imm_int(b, 1);
nir_ssa_def *add = nir_iadd(b, one, one);
/* This risks triggering removing add multiple times, which can segfault in
* nir_instr_remove for instructions with srcs. */
nir_ssa_def *add2 = nir_iadd(b, add, add);
nir_cursor c = nir_instr_free_and_dce(add2->parent_instr);
ASSERT_FALSE(shader_contains_def(add2));
ASSERT_FALSE(shader_contains_def(add));
ASSERT_FALSE(shader_contains_def(one));
ASSERT_TRUE(nir_cursors_equal(c, nir_before_block(nir_start_block(b->impl))));
nir_validate_shader(b->shader, "after remove_and_dce");
}
}