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

[Timothy Arceri]

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]);

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