<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 4, 2017 at 3:20 PM, Timothy Arceri <span dir="ltr"><<a href="mailto:timothy.arceri@collabora.com" target="_blank">timothy.arceri@collabora.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, 2016-12-12 at 19:39 -0800, Jason Ekstrand wrote:<br>
> ---<br>
>  src/compiler/nir/nir_remove_<wbr>dead_variables.c | 66<br>
> ++++++++++++++++++++++++----<br>
>  1 file changed, 58 insertions(+), 8 deletions(-)<br>
><br>
> diff --git a/src/compiler/nir/nir_remove_<wbr>dead_variables.c<br>
> b/src/compiler/nir/nir_remove_<wbr>dead_variables.c<br>
> index f7429eb..d22b7f5 100644<br>
> --- a/src/compiler/nir/nir_remove_<wbr>dead_variables.c<br>
> +++ b/src/compiler/nir/nir_remove_<wbr>dead_variables.c<br>
> @@ -31,9 +31,27 @@ static void<br>
>  add_var_use_intrinsic(nir_<wbr>intrinsic_instr *instr, struct set *live)<br>
>  {<br>
>     unsigned num_vars = nir_intrinsic_infos[instr-<br>
> >intrinsic].num_variables;<br>
> -   for (unsigned i = 0; i < num_vars; i++) {<br>
> -      nir_variable *var = instr->variables[i]->var;<br>
> -      _mesa_set_add(live, var);<br>
> +<br>
> +   switch (instr->intrinsic) {<br>
> +   case nir_intrinsic_copy_var:<br>
> +      _mesa_set_add(live, instr->variables[1]->var);<br>
> +      /* Fall through */<br>
> +   case nir_intrinsic_store_var: {<br>
> +      /* The first source in both copy_var and store_var is the<br>
> destination.<br>
> +       * If the variable is a local that never escapes the shader,<br>
> then we<br>
> +       * don't mark it as live for just a store.<br>
> +       */<br>
> +      nir_variable_mode mode = instr->variables[0]->var-><wbr>data.mode;<br>
> +      if (!(mode & (nir_var_local | nir_var_global |<br>
> nir_var_shared)))<br>
<br>
</div></div>So you have nir_var_shared here but I think you are missing the bit to<br>
add:<br>
<br>
   if (modes & nir_var_shared)<br>
      progress = remove_dead_vars(&shader-><wbr>shared, live) || progress;<br></blockquote><div><br></div><div>That needs to be its own patch, but sure.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Otherwise won't the var remain while the write instruction is removed?<br>
<div><div class="h5"><br>
<br>
> +         _mesa_set_add(live, instr->variables[0]->var);<br>
> +      break;<br>
> +   }<br>
> +<br>
> +   default:<br>
> +      for (unsigned i = 0; i < num_vars; i++) {<br>
> +         _mesa_set_add(live, instr->variables[i]->var);<br>
> +      }<br>
> +      break;<br>
>     }<br>
>  }<br>
>  <br>
> @@ -94,6 +112,31 @@ add_var_use_shader(nir_shader *shader, struct set<br>
> *live)<br>
>     }<br>
>  }<br>
>  <br>
> +static void<br>
> +remove_dead_var_writes(nir_<wbr>shader *shader, struct set *live)<br>
> +{<br>
> +   nir_foreach_function(<wbr>function, shader) {<br>
> +      if (!function->impl)<br>
> +         continue;<br>
> +<br>
> +      nir_foreach_block(<wbr>block, function->impl) {<br>
> +         nir_foreach_instr_<wbr>safe(instr, block) {<br>
> +            if (instr->type != nir_instr_type_intrinsic)<br>
> +               continue;<br>
> +<br>
> +            nir_intrinsic_<wbr>instr *intrin =<br>
> nir_instr_as_intrinsic(instr);<br>
> +            if (intrin->intrinsic != nir_intrinsic_copy_var &&<br>
> +                intrin-><wbr>intrinsic != nir_intrinsic_store_var)<br>
> +               continue;<br>
> +<br>
> +            /* Stores to dead variables need to be removed */<br>
> +            if (!_mesa_set_search(live, intrin->variables[0]->var))<br>
> +               nir_instr_<wbr>remove(instr);<br>
> +         }<br>
> +      }<br>
> +   }<br>
> +}<br>
> +<br>
>  static bool<br>
>  remove_dead_vars(struct exec_list *var_list, struct set *live)<br>
>  {<br>
> @@ -138,12 +181,19 @@ nir_remove_dead_variables(nir_<wbr>shader *shader,<br>
> nir_variable_mode modes)<br>
>     if (modes & nir_var_local) {<br>
>        nir_foreach_function(<wbr>function, shader) {<br>
>           if (function->impl) {<br>
> -            if (remove_dead_vars(&function-><wbr>impl->locals, live)) {<br>
> -               nir_metadata_<wbr>preserve(function->impl,<br>
> nir_metadata_block_index |<br>
> -                             <wbr>                        nir_<wbr>metadata_do<br>
> minance |<br>
> -                             <wbr>                        nir_<wbr>metadata_li<br>
> ve_ssa_defs);<br>
> +            if (remove_dead_vars(&function-><wbr>impl->locals, live))<br>
>                 progress = true;<br>
> -            }<br>
> +         }<br>
> +      }<br>
> +   }<br>
> +<br>
> +   if (progress) {<br>
> +      remove_dead_var_writes(<wbr>shader, live);<br>
<br>
</div></div>The vars are conditional on modes passed to nir_remove_dead_variables()<br>
dont we need this here too? Otherwise we will remove the write<br>
instruction but not the var.<br></blockquote><div><br></div>Hrm... Yes.  I'm thinking that we probably want to set the variable mode to 0 when we delete it and use that to decide when to remove a write.  That way we never run into this problem in the future.<br></div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
> +<br>
> +      nir_foreach_function(<wbr>function, shader) {<br>
> +         if (function->impl) {<br>
> +            nir_metadata_<wbr>preserve(function->impl,<br>
> nir_metadata_block_index |<br>
> +                             <wbr>                     nir_<wbr>metadata_domin<br>
> ance);<br>
>           }<br>
>        }<br>
>     }<br>
</div></div></blockquote></div><br></div></div>