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

[Timothy Arceri]

On 11/10/18 6:59 pm, Samuel Pitoiset wrote:
> 
> 
> 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()?

nir_lower_io_arrays_to_elements() will lower the inputs unless we set 
always_active_io there also (currently it will only be set on the 
matching output).

> 
>> 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,
>>>