[Mesa-dev] [PATCH] nir: fix nir_remove_unused_varyings()

[Dylan Baker]

Hi Tim,

I had to make a couple of small tweaks to get this to apply against 19.0 (namely
that the glsl_type_is_struct -> glsl_type_is_struct_or_ifc doesn't exist in
19.0), could you take a look at the patch in the staging/19.0 branch and let me
know if it looks okay?

Thanks,
Dylan

Quoting Timothy Arceri (2019-04-24 18:17:42)
> We were only setting the used mask for the first component of a
> varying. Since the linking opts split vectors into scalars this
> has mostly worked ok.
> 
> However this causes an issue where for example if we split a
> struct on one side of the interface but not the other, then we
> can possibly end up removing the first components on the side
> that was split and then incorrectly remove the whole struct
> on the other side of the varying.
> 
> With this change we simply mark all 4 components for each slot
> used by a struct. We could possibly make this more fine gained
> but that would require a more complex change.
> 
> This fixes a bug in Strange Brigade on RADV when tessellation
> is enabled, all credit goes to Samuel Pitoiset for tracking down
> the cause of the bug.
> 
> Fixes: f1eb5e639997 ("nir: add component level support to remove_unused_io_vars()")
> ---
>  src/compiler/nir/nir_linking_helpers.c | 51 +++++++++++++++++---------
>  1 file changed, 33 insertions(+), 18 deletions(-)
> 
> diff --git a/src/compiler/nir/nir_linking_helpers.c b/src/compiler/nir/nir_linking_helpers.c
> index f4494c78f98..ef0f94baf47 100644
> --- a/src/compiler/nir/nir_linking_helpers.c
> +++ b/src/compiler/nir/nir_linking_helpers.c
> @@ -59,6 +59,15 @@ get_variable_io_mask(nir_variable *var, gl_shader_stage stage)
>     return ((1ull << slots) - 1) << location;
>  }
>  
> +static uint8_t
> +get_num_components(nir_variable *var)
> +{
> +   if (glsl_type_is_struct_or_ifc(glsl_without_array(var->type)))
> +      return 4;
> +
> +   return glsl_get_vector_elements(glsl_without_array(var->type));
> +}
> +
>  static void
>  tcs_add_output_reads(nir_shader *shader, uint64_t *read, uint64_t *patches_read)
>  {
> @@ -80,12 +89,14 @@ tcs_add_output_reads(nir_shader *shader, uint64_t *read, uint64_t *patches_read)
>                 continue;
>  
>              nir_variable *var = nir_deref_instr_get_variable(deref);
> -            if (var->data.patch) {
> -               patches_read[var->data.location_frac] |=
> -                  get_variable_io_mask(var, shader->info.stage);
> -            } else {
> -               read[var->data.location_frac] |=
> -                  get_variable_io_mask(var, shader->info.stage);
> +            for (unsigned i = 0; i < get_num_components(var); i++) {
> +               if (var->data.patch) {
> +                  patches_read[var->data.location_frac + i] |=
> +                     get_variable_io_mask(var, shader->info.stage);
> +               } else {
> +                  read[var->data.location_frac + i] |=
> +                     get_variable_io_mask(var, shader->info.stage);
> +               }
>              }
>           }
>        }
> @@ -161,22 +172,26 @@ nir_remove_unused_varyings(nir_shader *producer, nir_shader *consumer)
>     uint64_t patches_read[4] = { 0 }, patches_written[4] = { 0 };
>  
>     nir_foreach_variable(var, &producer->outputs) {
> -      if (var->data.patch) {
> -         patches_written[var->data.location_frac] |=
> -            get_variable_io_mask(var, producer->info.stage);
> -      } else {
> -         written[var->data.location_frac] |=
> -            get_variable_io_mask(var, producer->info.stage);
> +      for (unsigned i = 0; i < get_num_components(var); i++) {
> +         if (var->data.patch) {
> +            patches_written[var->data.location_frac + i] |=
> +               get_variable_io_mask(var, producer->info.stage);
> +         } else {
> +            written[var->data.location_frac + i] |=
> +               get_variable_io_mask(var, producer->info.stage);
> +         }
>        }
>     }
>  
>     nir_foreach_variable(var, &consumer->inputs) {
> -      if (var->data.patch) {
> -         patches_read[var->data.location_frac] |=
> -            get_variable_io_mask(var, consumer->info.stage);
> -      } else {
> -         read[var->data.location_frac] |=
> -            get_variable_io_mask(var, consumer->info.stage);
> +      for (unsigned i = 0; i < get_num_components(var); i++) {
> +         if (var->data.patch) {
> +            patches_read[var->data.location_frac + i] |=
> +               get_variable_io_mask(var, consumer->info.stage);
> +         } else {
> +            read[var->data.location_frac + i] |=
> +               get_variable_io_mask(var, consumer->info.stage);
> +         }
>        }
>     }
>  
> -- 
> 2.20.1
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190425/e34ce8de/attachment.sig>