[Mesa-dev] [PATCH v2] nir: fix compacting varyings when XFB outputs are present

[Samuel Pitoiset]

On 10/10/18 11:46 PM, Timothy Arceri wrote:
> 
> 
> On 11/10/18 1:46 am, Samuel Pitoiset wrote:
>> We shouldn't try to compact any varyings known as always
>> active IO, especially XFB outputs. For example, if one
>> component of an xfb output is also used as input varying
>> in the next stage, it shouldn't be compacted.
>>
>> This small helper just marks all XFB varyings as always_active_io
>> in the consumer to not compact them.
>>
>> v2: add a little helper
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>>   src/compiler/nir/nir_linking_helpers.c | 33 ++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/src/compiler/nir/nir_linking_helpers.c 
>> b/src/compiler/nir/nir_linking_helpers.c
>> index 88014e9a1d..433729bd79 100644
>> --- a/src/compiler/nir/nir_linking_helpers.c
>> +++ b/src/compiler/nir/nir_linking_helpers.c
>> @@ -480,6 +480,36 @@ compact_components(nir_shader *producer, 
>> nir_shader *consumer, uint8_t *comps,
>>                                 &producer->info.outputs_read);
>>   }
>> +/* Mark XFB varyings as always_active_io in the consumer to not 
>> compact them. */
>> +static void
>> +link_xfb_varyings(nir_shader *producer, nir_shader *consumer)
>> +{
>> +   nir_variable *input_vars[32] = {};
> 
>        nir_variable *input_vars[MAX_VARYING] = {};
> 
>> +
>> +   nir_foreach_variable(var, &consumer->inputs) {
>> +      if (var->data.location >= VARYING_SLOT_VAR0 &&
>> +          var->data.location - VARYING_SLOT_VAR0 < 32) {
> 
>            var->data.location - VARYING_SLOT_VAR0 < MAX_VARYING) {
> 
> 
>> +
>> +         unsigned location = var->data.location - VARYING_SLOT_VAR0;
>> +         input_vars[location] = var;
>> +      }
>> +   }
>> +
>> +   nir_foreach_variable(var, &producer->outputs) {
>> +      if (var->data.location >= VARYING_SLOT_VAR0 &&
>> +          var->data.location - VARYING_SLOT_VAR0 < 32) {
> 
>            var->data.location - VARYING_SLOT_VAR0 < MAX_VARYING) {
> 
>> +
>> +         if (!var->data.explicit_xfb_buffer)
>> +            continue;
>> +
>> +         unsigned location = var->data.location - VARYING_SLOT_VAR0;
>> +         if (input_vars[location]) {
>> +            input_vars[location]->data.always_active_io = true;
>> +         }
>> +      }
>> +   }
>> +}
>> +
>>   /* We assume that this has been called more-or-less directly after
>>    * remove_unused_varyings.  At this point, all of the varyings that we
>>    * aren't going to be using have been completely removed and the
>> @@ -501,6 +531,9 @@ nir_compact_varyings(nir_shader *producer, 
>> nir_shader *consumer,
>>      uint8_t interp_type[32] = {0};
>>      uint8_t interp_loc[32] = {0};
> 
> Hmm, looks like I should fix up these magic numbers also. I was planning 
> on changing this code when adding patch support (which I still haven't 
> got round to).
> 
>> +   if (producer->info.has_transform_feedback_varyings)
>> +      link_xfb_varyings(producer, consumer);
> 
> I think you need to expose link_xfb_varyings() externally and call it in 
> radv_link_shaders() before nir_lower_io_arrays_to_elements()
> 
> e.g
> 
>     if (ordered_shaders[i]->info.has_transform_feedback_varyings)
>        nir_link_xfb_varyings(ordered_shaders[i], ordered_shaders[i - 1]);
> 

nir_lower_io_arrays_to_elements() shouldn't lower anything because it 
checks for always_active_io. Why don't call it just before 
nir_compact_varyings() and after nir_remove_unused_varyings()?

> With these changes this patch is:
> 
> Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>
> 
> 
>> +
>>      get_slot_component_masks_and_interp_types(&producer->outputs, comps,
>>                                                interp_type, interp_loc,
>>                                                producer->info.stage,
>>