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

[Timothy Arceri]

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