Re: [Mesa-dev] [PATCH 2/9] nir: insert ssa_undef instructions when cleanup up defs/uses
On Sat, May 9, 2015 at 1:52 AM, Jason Ekstrand wrote: > On Fri, May 8, 2015 at 10:03 PM, Connor Abbott wrote: >> The point of cleanup_defs_uses() is to make an instruction safe to >> remove by removing any references that the rest of the shader may have >> to it. Previously, it was handling register use/def sets and removing >> the instruction from the use sets of any SSA sources it had, but if the >> instruction defined an SSA value that was used by other instructions it >> wasn't doing anything. This was ok, since we were always careful to make >> sure that no removed instruction ever had any uses, but now we want to >> start removing unreachable instruction which might still be used in >> reachable parts of the code. In that case, the value that any uses get >> is undefined since the instruction never actually executes, so we can >> just replace the instruction with an ssa_undef_instr. >> >> Signed-off-by: Connor Abbott >> --- >> src/glsl/nir/nir.c | 47 ++- >> 1 file changed, 34 insertions(+), 13 deletions(-) >> >> diff --git a/src/glsl/nir/nir.c b/src/glsl/nir/nir.c >> index f03e80a..362ac30 100644 >> --- a/src/glsl/nir/nir.c >> +++ b/src/glsl/nir/nir.c >> @@ -1206,26 +1206,29 @@ stitch_blocks(nir_block *before, nir_block *after) >> } >> >> static void >> -remove_defs_uses(nir_instr *instr); >> +remove_defs_uses(nir_instr *instr, nir_function_impl *impl); >> >> static void >> -cleanup_cf_node(nir_cf_node *node) >> +cleanup_cf_node(nir_cf_node *node, nir_function_impl *impl) >> { >> switch (node->type) { >> case nir_cf_node_block: { >>nir_block *block = nir_cf_node_as_block(node); >>/* We need to walk the instructions and clean up defs/uses */ >> - nir_foreach_instr(block, instr) >> - remove_defs_uses(instr); >> + nir_foreach_instr(block, instr) { >> + if (instr->type == nir_instr_type_jump) >> +handle_remove_jump(block, nir_instr_as_jump(instr)->type); >> + remove_defs_uses(instr, impl); >> + } > > This looks like an unrelated change. Maybe split that out? The rest > of the patch looks plausible. > --Jason Yeah, right... part of this change is just passing impl through to remove_defs_uses() so it doesn't have to recompute it, but the other part is fixing another bug where we would forget to fixup the successors/predecessors when removing jumps. I'll split out the latter part. > >>break; >> } >> >> case nir_cf_node_if: { >>nir_if *if_stmt = nir_cf_node_as_if(node); >>foreach_list_typed(nir_cf_node, child, node, &if_stmt->then_list) >> - cleanup_cf_node(child); >> + cleanup_cf_node(child, impl); >>foreach_list_typed(nir_cf_node, child, node, &if_stmt->else_list) >> - cleanup_cf_node(child); >> + cleanup_cf_node(child, impl); >> >>list_del(&if_stmt->condition.use_link); >>break; >> @@ -1234,13 +1237,12 @@ cleanup_cf_node(nir_cf_node *node) >> case nir_cf_node_loop: { >>nir_loop *loop = nir_cf_node_as_loop(node); >>foreach_list_typed(nir_cf_node, child, node, &loop->body) >> - cleanup_cf_node(child); >> + cleanup_cf_node(child, impl); >>break; >> } >> case nir_cf_node_function: { >> - nir_function_impl *impl = nir_cf_node_as_function(node); >>foreach_list_typed(nir_cf_node, child, node, &impl->body) >> - cleanup_cf_node(child); >> + cleanup_cf_node(child, impl); >>break; >> } >> default: >> @@ -1254,6 +1256,8 @@ nir_cf_node_remove(nir_cf_node *node) >> nir_function_impl *impl = nir_cf_node_get_function(node); >> nir_metadata_preserve(impl, nir_metadata_none); >> >> + cleanup_cf_node(node, impl); >> + >> if (node->type == nir_cf_node_block) { >>/* >> * Basic blocks can't really be removed by themselves, since they act >> as >> @@ -1275,8 +1279,6 @@ nir_cf_node_remove(nir_cf_node *node) >>exec_node_remove(&node->node); >>stitch_blocks(before_block, after_block); >> } >> - >> - cleanup_cf_node(node); >> } >> >> static bool >> @@ -1443,16 +1445,35 @@ remove_def_cb(nir_dest *dest, void *state) >> return true; >> } >> >> +static bool >> +remove_ssa_def_cb(nir_ssa_def *def, void *state) >> +{ >> + nir_function_impl *impl = state; >> + nir_shader *shader = impl->overload->function->shader; >> + >> + if (!list_empty(&def->uses) || !list_empty(&def->if_uses)) { >> + nir_ssa_undef_instr *undef = >> + nir_ssa_undef_instr_create(shader, def->num_components); >> + nir_instr_insert_before_cf_list(&impl->body, &undef->instr); >> + nir_ssa_def_rewrite_uses(def, nir_src_for_ssa(&undef->def), shader); >> + } >> + >> + return true; >> +} >> + >> + >> static void >> -remove_defs_uses(nir_instr *instr) >> +remove_defs_uses(nir_instr *instr, nir_function_impl *impl) >> { >> nir_foreach_dest(instr, remove_def_cb, instr);
Re: [Mesa-dev] [PATCH 2/9] nir: insert ssa_undef instructions when cleanup up defs/uses
On Fri, May 8, 2015 at 10:03 PM, Connor Abbott wrote: > The point of cleanup_defs_uses() is to make an instruction safe to > remove by removing any references that the rest of the shader may have > to it. Previously, it was handling register use/def sets and removing > the instruction from the use sets of any SSA sources it had, but if the > instruction defined an SSA value that was used by other instructions it > wasn't doing anything. This was ok, since we were always careful to make > sure that no removed instruction ever had any uses, but now we want to > start removing unreachable instruction which might still be used in > reachable parts of the code. In that case, the value that any uses get > is undefined since the instruction never actually executes, so we can > just replace the instruction with an ssa_undef_instr. > > Signed-off-by: Connor Abbott > --- > src/glsl/nir/nir.c | 47 ++- > 1 file changed, 34 insertions(+), 13 deletions(-) > > diff --git a/src/glsl/nir/nir.c b/src/glsl/nir/nir.c > index f03e80a..362ac30 100644 > --- a/src/glsl/nir/nir.c > +++ b/src/glsl/nir/nir.c > @@ -1206,26 +1206,29 @@ stitch_blocks(nir_block *before, nir_block *after) > } > > static void > -remove_defs_uses(nir_instr *instr); > +remove_defs_uses(nir_instr *instr, nir_function_impl *impl); > > static void > -cleanup_cf_node(nir_cf_node *node) > +cleanup_cf_node(nir_cf_node *node, nir_function_impl *impl) > { > switch (node->type) { > case nir_cf_node_block: { >nir_block *block = nir_cf_node_as_block(node); >/* We need to walk the instructions and clean up defs/uses */ > - nir_foreach_instr(block, instr) > - remove_defs_uses(instr); > + nir_foreach_instr(block, instr) { > + if (instr->type == nir_instr_type_jump) > +handle_remove_jump(block, nir_instr_as_jump(instr)->type); > + remove_defs_uses(instr, impl); > + } This looks like an unrelated change. Maybe split that out? The rest of the patch looks plausible. --Jason >break; > } > > case nir_cf_node_if: { >nir_if *if_stmt = nir_cf_node_as_if(node); >foreach_list_typed(nir_cf_node, child, node, &if_stmt->then_list) > - cleanup_cf_node(child); > + cleanup_cf_node(child, impl); >foreach_list_typed(nir_cf_node, child, node, &if_stmt->else_list) > - cleanup_cf_node(child); > + cleanup_cf_node(child, impl); > >list_del(&if_stmt->condition.use_link); >break; > @@ -1234,13 +1237,12 @@ cleanup_cf_node(nir_cf_node *node) > case nir_cf_node_loop: { >nir_loop *loop = nir_cf_node_as_loop(node); >foreach_list_typed(nir_cf_node, child, node, &loop->body) > - cleanup_cf_node(child); > + cleanup_cf_node(child, impl); >break; > } > case nir_cf_node_function: { > - nir_function_impl *impl = nir_cf_node_as_function(node); >foreach_list_typed(nir_cf_node, child, node, &impl->body) > - cleanup_cf_node(child); > + cleanup_cf_node(child, impl); >break; > } > default: > @@ -1254,6 +1256,8 @@ nir_cf_node_remove(nir_cf_node *node) > nir_function_impl *impl = nir_cf_node_get_function(node); > nir_metadata_preserve(impl, nir_metadata_none); > > + cleanup_cf_node(node, impl); > + > if (node->type == nir_cf_node_block) { >/* > * Basic blocks can't really be removed by themselves, since they act > as > @@ -1275,8 +1279,6 @@ nir_cf_node_remove(nir_cf_node *node) >exec_node_remove(&node->node); >stitch_blocks(before_block, after_block); > } > - > - cleanup_cf_node(node); > } > > static bool > @@ -1443,16 +1445,35 @@ remove_def_cb(nir_dest *dest, void *state) > return true; > } > > +static bool > +remove_ssa_def_cb(nir_ssa_def *def, void *state) > +{ > + nir_function_impl *impl = state; > + nir_shader *shader = impl->overload->function->shader; > + > + if (!list_empty(&def->uses) || !list_empty(&def->if_uses)) { > + nir_ssa_undef_instr *undef = > + nir_ssa_undef_instr_create(shader, def->num_components); > + nir_instr_insert_before_cf_list(&impl->body, &undef->instr); > + nir_ssa_def_rewrite_uses(def, nir_src_for_ssa(&undef->def), shader); > + } > + > + return true; > +} > + > + > static void > -remove_defs_uses(nir_instr *instr) > +remove_defs_uses(nir_instr *instr, nir_function_impl *impl) > { > nir_foreach_dest(instr, remove_def_cb, instr); > nir_foreach_src(instr, remove_use_cb, instr); > + nir_foreach_ssa_def(instr, remove_ssa_def_cb, impl); > } > > void nir_instr_remove(nir_instr *instr) > { > - remove_defs_uses(instr); > + nir_function_impl *impl = > nir_cf_node_get_function(&instr->block->cf_node); > + remove_defs_uses(instr, impl); > exec_node_remove(&instr->node); > > if (instr->type == nir_instr_type_jump) { > -- > 2.1.0 > >
[Mesa-dev] [PATCH 2/9] nir: insert ssa_undef instructions when cleanup up defs/uses
The point of cleanup_defs_uses() is to make an instruction safe to remove by removing any references that the rest of the shader may have to it. Previously, it was handling register use/def sets and removing the instruction from the use sets of any SSA sources it had, but if the instruction defined an SSA value that was used by other instructions it wasn't doing anything. This was ok, since we were always careful to make sure that no removed instruction ever had any uses, but now we want to start removing unreachable instruction which might still be used in reachable parts of the code. In that case, the value that any uses get is undefined since the instruction never actually executes, so we can just replace the instruction with an ssa_undef_instr. Signed-off-by: Connor Abbott --- src/glsl/nir/nir.c | 47 ++- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/src/glsl/nir/nir.c b/src/glsl/nir/nir.c index f03e80a..362ac30 100644 --- a/src/glsl/nir/nir.c +++ b/src/glsl/nir/nir.c @@ -1206,26 +1206,29 @@ stitch_blocks(nir_block *before, nir_block *after) } static void -remove_defs_uses(nir_instr *instr); +remove_defs_uses(nir_instr *instr, nir_function_impl *impl); static void -cleanup_cf_node(nir_cf_node *node) +cleanup_cf_node(nir_cf_node *node, nir_function_impl *impl) { switch (node->type) { case nir_cf_node_block: { nir_block *block = nir_cf_node_as_block(node); /* We need to walk the instructions and clean up defs/uses */ - nir_foreach_instr(block, instr) - remove_defs_uses(instr); + nir_foreach_instr(block, instr) { + if (instr->type == nir_instr_type_jump) +handle_remove_jump(block, nir_instr_as_jump(instr)->type); + remove_defs_uses(instr, impl); + } break; } case nir_cf_node_if: { nir_if *if_stmt = nir_cf_node_as_if(node); foreach_list_typed(nir_cf_node, child, node, &if_stmt->then_list) - cleanup_cf_node(child); + cleanup_cf_node(child, impl); foreach_list_typed(nir_cf_node, child, node, &if_stmt->else_list) - cleanup_cf_node(child); + cleanup_cf_node(child, impl); list_del(&if_stmt->condition.use_link); break; @@ -1234,13 +1237,12 @@ cleanup_cf_node(nir_cf_node *node) case nir_cf_node_loop: { nir_loop *loop = nir_cf_node_as_loop(node); foreach_list_typed(nir_cf_node, child, node, &loop->body) - cleanup_cf_node(child); + cleanup_cf_node(child, impl); break; } case nir_cf_node_function: { - nir_function_impl *impl = nir_cf_node_as_function(node); foreach_list_typed(nir_cf_node, child, node, &impl->body) - cleanup_cf_node(child); + cleanup_cf_node(child, impl); break; } default: @@ -1254,6 +1256,8 @@ nir_cf_node_remove(nir_cf_node *node) nir_function_impl *impl = nir_cf_node_get_function(node); nir_metadata_preserve(impl, nir_metadata_none); + cleanup_cf_node(node, impl); + if (node->type == nir_cf_node_block) { /* * Basic blocks can't really be removed by themselves, since they act as @@ -1275,8 +1279,6 @@ nir_cf_node_remove(nir_cf_node *node) exec_node_remove(&node->node); stitch_blocks(before_block, after_block); } - - cleanup_cf_node(node); } static bool @@ -1443,16 +1445,35 @@ remove_def_cb(nir_dest *dest, void *state) return true; } +static bool +remove_ssa_def_cb(nir_ssa_def *def, void *state) +{ + nir_function_impl *impl = state; + nir_shader *shader = impl->overload->function->shader; + + if (!list_empty(&def->uses) || !list_empty(&def->if_uses)) { + nir_ssa_undef_instr *undef = + nir_ssa_undef_instr_create(shader, def->num_components); + nir_instr_insert_before_cf_list(&impl->body, &undef->instr); + nir_ssa_def_rewrite_uses(def, nir_src_for_ssa(&undef->def), shader); + } + + return true; +} + + static void -remove_defs_uses(nir_instr *instr) +remove_defs_uses(nir_instr *instr, nir_function_impl *impl) { nir_foreach_dest(instr, remove_def_cb, instr); nir_foreach_src(instr, remove_use_cb, instr); + nir_foreach_ssa_def(instr, remove_ssa_def_cb, impl); } void nir_instr_remove(nir_instr *instr) { - remove_defs_uses(instr); + nir_function_impl *impl = nir_cf_node_get_function(&instr->block->cf_node); + remove_defs_uses(instr, impl); exec_node_remove(&instr->node); if (instr->type == nir_instr_type_jump) { -- 2.1.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev