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

[Dylan Baker]

Thanks!

Quoting Timothy Arceri (2019-04-25 15:36:44)
> On 26/4/19 6:50 am, Dylan Baker wrote:
> > 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?
> 
> Looks good to me. Thanks!
> 
> > 
> > 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/55b2ac5a/attachment.sig>