[Mesa-dev] [PATCH 5/9] nir: Add a local dead write vars removal pass

[Caio Marcelo de Oliveira Filho]

(Disregard the incomplete mail, still adapting to notmuch-emacs).

Jason Ekstrand <jason at jlekstrand.net> writes:

>> +static nir_deref_path *
>> +get_path(struct state *state, nir_deref_instr *deref)
>> +{
>> +   struct hash_entry *entry = _mesa_hash_table_search(state->paths,
>> deref);
>> +   if (!entry) {
>> +      nir_deref_path *path = linear_zalloc_child(state->path_lin_ctx,
>> sizeof(nir_deref_path));
>> +      nir_deref_path_init(path, deref, state->mem_ctx);
>> +      _mesa_hash_table_insert(state->paths, deref, path);
>> +      return path;
>> +   } else {
>> +      return entry->data;
>> +   }
>> +}
>>
>
> Do you have any proof that this actually helps?  The deref_path stuff was
> designed to be put on the stack and absolutely as efficient as possible.
> In the common case of a deref chain with only a couple of elements, I would
> expect to actually be more work to look it up in a hash table.

Storing those makes more sense if you read the next commit (the
"global").  Since I've created the "local" commit as an aid for
reviewing (perhaps a failure), I did not wanted to change it too much.

When I wrote there were some savings when measuring executions with
perf.

(...)

>> +static bool
>> +remove_dead_write_vars_local(struct state *state, nir_block *block)
>> +{
>> +   bool progress = false;
>> +
>> +   struct util_dynarray unused_writes;
>> +   util_dynarray_init(&unused_writes, state->mem_ctx);
>> +
>> +   nir_foreach_instr_safe(instr, block) {
>>
>
> It wouldn't hurt to add a case for call instructions which does a barrier
> on everything I mentioned below as well as globals and locals.

Makes sense.  But I don't get locals are affect?  Is this to cover the
parameters being passed to the call?

>
>> +      if (instr->type != nir_instr_type_intrinsic)
>> +         continue;
>> +
>> +      nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);
>> +      switch (intrin->intrinsic) {
>> +      case nir_intrinsic_barrier:
>> +      case nir_intrinsic_memory_barrier: {
>> +         nir_variable_mode modes = ~(nir_var_local | nir_var_global |
>> +                                     nir_var_shader_in | nir_var_uniform);
>>
>
> The only thing a barrier like this affects is shared, storage, and output.
> Locals and globals can't cross between shader channels so there's no reason
> to do anything with them on a barrier.  For inputs and uniforms, they're
> never written anyway so there's no point in doing anything with them on a
> barrier.

This came from previous code, but except for "system values" it seems to
do the right thing (note the ~).  Is the suggestion to use a "positive"
enumeration, e.g.

    clear_unused_for_modes(&unused_writes, nir_var_shader_out |
                                           nir_var_shader_storage |
                                           nir_var_shared);



Thanks,
Caio