<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Aug 27, 2018 at 4:55 PM Caio Marcelo de Oliveira Filho <<a href="mailto:caio.oliveira@intel.com">caio.oliveira@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">(Disregard the incomplete mail, still adapting to notmuch-emacs).<br>
<br>
Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> writes:<br>
<br>
>> +static nir_deref_path *<br>
>> +get_path(struct state *state, nir_deref_instr *deref)<br>
>> +{<br>
>> +   struct hash_entry *entry = _mesa_hash_table_search(state->paths,<br>
>> deref);<br>
>> +   if (!entry) {<br>
>> +      nir_deref_path *path = linear_zalloc_child(state->path_lin_ctx,<br>
>> sizeof(nir_deref_path));<br>
>> +      nir_deref_path_init(path, deref, state->mem_ctx);<br>
>> +      _mesa_hash_table_insert(state->paths, deref, path);<br>
>> +      return path;<br>
>> +   } else {<br>
>> +      return entry->data;<br>
>> +   }<br>
>> +}<br>
>><br>
><br>
> Do you have any proof that this actually helps?  The deref_path stuff was<br>
> designed to be put on the stack and absolutely as efficient as possible.<br>
> In the common case of a deref chain with only a couple of elements, I would<br>
> expect to actually be more work to look it up in a hash table.<br>
<br>
Storing those makes more sense if you read the next commit (the<br>
"global").  Since I've created the "local" commit as an aid for<br>
reviewing (perhaps a failure), I did not wanted to change it too much.<br>
<br>
When I wrote there were some savings when measuring executions with<br>
perf.<br>
<br>
(...)<br>
<br>
>> +static bool<br>
>> +remove_dead_write_vars_local(struct state *state, nir_block *block)<br>
>> +{<br>
>> +   bool progress = false;<br>
>> +<br>
>> +   struct util_dynarray unused_writes;<br>
>> +   util_dynarray_init(&unused_writes, state->mem_ctx);<br>
>> +<br>
>> +   nir_foreach_instr_safe(instr, block) {<br>
>><br>
><br>
> It wouldn't hurt to add a case for call instructions which does a barrier<br>
> on everything I mentioned below as well as globals and locals.<br>
<br>
Makes sense.  But I don't get locals are affect?  Is this to cover the<br>
parameters being passed to the call?<br></blockquote><div><br></div><div>Because a deref to a local might be passed in as a parameter.  This is the way pass-by-reference works for SPIR-V.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
>> +      if (instr->type != nir_instr_type_intrinsic)<br>
>> +         continue;<br>
>> +<br>
>> +      nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);<br>
>> +      switch (intrin->intrinsic) {<br>
>> +      case nir_intrinsic_barrier:<br>
>> +      case nir_intrinsic_memory_barrier: {<br>
>> +         nir_variable_mode modes = ~(nir_var_local | nir_var_global |<br>
>> +                                     nir_var_shader_in | nir_var_uniform);<br>
>><br>
><br>
> The only thing a barrier like this affects is shared, storage, and output.<br>
> Locals and globals can't cross between shader channels so there's no reason<br>
> to do anything with them on a barrier.  For inputs and uniforms, they're<br>
> never written anyway so there's no point in doing anything with them on a<br>
> barrier.<br>
<br>
This came from previous code, but except for "system values" it seems to<br>
do the right thing (note the ~).  Is the suggestion to use a "positive"<br>
enumeration, e.g.<br></blockquote><div><br></div><div>Drp... I completely missed the ~.  Ignore my comment.  Yeah, we could throw in system values. :D <br></div></div></div>