[Mesa-dev] [PATCH 4/8] nir: Properly invalidate metadata in nir_remove_dead_variables().

[Eduardo Lima Mitev]

On 11/03/2015 09:31 AM, Kenneth Graunke wrote:
> We can't preserve dominance or live variable information.
> 
> This also begs the question: what about globals?  Metadata only exists
> at the nir_function_impl level, so it would seem there is no metadata
> about global variables for us to invalidate.
> 

Shouldn't globals be always lowered to locals prior to invoking this
pass? That's what i965 does. Are there other drivers using nir, that
deal with global vars?

> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/glsl/nir/nir_remove_dead_variables.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/glsl/nir/nir_remove_dead_variables.c b/src/glsl/nir/nir_remove_dead_variables.c
> index d6783e7..77b6c13 100644
> --- a/src/glsl/nir/nir_remove_dead_variables.c
> +++ b/src/glsl/nir/nir_remove_dead_variables.c
> @@ -126,8 +126,13 @@ nir_remove_dead_variables(nir_shader *shader)
>     progress = remove_dead_vars(&shader->globals, live) || progress;

At least in i965, where nir_lower_global_vars_to_local() runs prior to
this pass, only dead global variables can reach this point. So metadata
seems irrelevant here.

I tested this quickly with the following patch, and saw no new crashes
running several tests:

diff --git a/src/glsl/nir/nir_remove_dead_variables.c
b/src/glsl/nir/nir_remove_dead_variables.c
index d6783e7..7db48eb 100644
--- a/src/glsl/nir/nir_remove_dead_variables.c
+++ b/src/glsl/nir/nir_remove_dead_variables.c
@@ -124,6 +124,7 @@ nir_remove_dead_variables(nir_shader *shader)
    add_var_use_shader(shader, live);

    progress = remove_dead_vars(&shader->globals, live) || progress;
+   assert(exec_list_length(&shader->globals) == 0);

    nir_foreach_overload(shader, overload) {
       if (overload->impl)

Eduardo

>  
>     nir_foreach_overload(shader, overload) {
> -      if (overload->impl)
> -         progress = remove_dead_vars(&overload->impl->locals, live) || progress;
> +      if (overload->impl) {
> +         if (remove_dead_vars(&overload->impl->locals, live)) {
> +            nir_metadata_preserve(overload->impl, nir_metadata_block_index |
> +                                                  nir_metadata_dominance);
> +            progress = true;
> +         }
> +      }
>     }
>  
>     _mesa_set_destroy(live, NULL);
>