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

[Samuel Pitoiset]

Reviewed-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>

Thanks fo the fix Tim!

On 4/25/19 3:17 AM, Timothy Arceri wrote:
> 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);
> +         }
>         }
>      }
>