[Mesa-dev] [PATCH 2/9] nir: insert ssa_undef instructions when cleanup up defs/uses

[Connor Abbott]

On Sat, May 9, 2015 at 1:52 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Fri, May 8, 2015 at 10:03 PM, Connor Abbott <cwabbott0 at gmail.com> 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 <cwabbott0 at gmail.com>
>> ---
>>  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);
>>     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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev