[Mesa-dev] [PATCH 1/2] glsl: Don't remove XFB-only varyings.

[Timothy Arceri]

On Sun, 2016-04-03 at 21:30 -0700, Kenneth Graunke wrote:
> On Sunday, April 3, 2016 9:34:05 PM PDT Timothy Arceri wrote:
> > 
> > On Sun, 2016-04-03 at 01:37 -0700, Kenneth Graunke wrote:
> > > 
> > > Consider the case of linking a program with both a vertex and
> > > fragment
> > > shader.  The VS may compute output varyings that are intended for
> > > transform feedback, and not read by the fragment shader.
> > > 
> > > In this case, var->data.is_unmatched_generic_inout will be true,
> > > but we still cannot eliminate the varyings.  We need to also
> > > check
> > > !var->data.is_xfb_only.
> > > 
> > > Fixes failures in ES31-CTS.gpu_shader5.fma_precision_*, which
> > > happen
> > > to use transform feedback in a way we apparently hadn't seen
> > > before.
> > Hmm ... are you sure there is not something else going wrong here
> > somewhere? is_xfb_only is set in the same code block that should
> > change
> > is_unmatched_generic_inout to false.
> > 
> >       if (matched_candidate->toplevel_var-
> > >data.is_unmatched_generic_inout) 
> {
> > 
> >          matched_candidate->toplevel_var->data.is_xfb_only = 1;
> >          matches.record(matched_candidate->toplevel_var, NULL);
> >       }
> > 
> > matches.record() should set is_unmatched_generic_inout to false and
> > setup everything else needed for xfb. Seems like there is another
> > bug
> > here somewhere.
> The shader actually contains:
> 
>     layout(location = 0) out flat int outTextureSize;
> 
> So it triggers the very first condition of matches.record():
> 
>    if ((producer_var && (!producer_var-
> >data.is_unmatched_generic_inout ||
>        producer_var->data.explicit_location)) ||
>        (consumer_var && (!consumer_var-
> >data.is_unmatched_generic_inout ||
>        consumer_var->data.explicit_location))) {
>       /* Either a location already exists for this variable (since it
> is part
>        * of fixed functionality), or it has already been recorded as
> part of a
>        * previous match.
>        */
>       return;
>    }
> 
> and so it returns early.  No match is recorded, and it never sets
> producer_var->data.is_unmatched_generic_inout = 0.
> 
> Do you think we should do some of it anyway?

I've taken another look, and I think this is ok. The reason for
record() is to 1. mark something as needing to be assigned a location
and 2. setting the interpolation type to flat for packing. Neither of
these should be needed in this case. The level of abstraction in this
code makes it hard to folow at times :P

Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>
> 
> --Ken