[Mesa-dev] [PATCH] glsl/linker: outputs in the same location must share interpolation

[Iago Toral]

On Thu, 2017-10-19 at 17:14 +1100, Timothy Arceri wrote:
> 
> On 19/10/17 16:57, Iago Toral Quiroga wrote:
> >  From ARB_enhanced_layouts:
> > 
> > "[...]when location aliasing, the aliases sharing the location
> >   must have the same underlying numerical type (floating-point or
> >   integer) and the same auxiliary storage and
> >   interpolation qualification.[...]"
> > 
> > Add code to the linker to validate that aliased locations do
> > have the same interpolation.
> > 
> > Fixes:
> > KHR-
> > GL45.enhanced_layouts.varying_location_aliasing_with_mixed_interpol
> > ation
> > ---
> >   src/compiler/glsl/link_varyings.cpp | 35
> > +++++++++++++++++++++++++++++++++++
> >   1 file changed, 35 insertions(+)
> > 
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index 69c92bf53b..c888635e82 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -459,6 +459,41 @@ cross_validate_outputs_to_inputs(struct
> > gl_context *ctx,
> >   
> >            while (idx < slot_limit) {
> >               unsigned i = var->data.location_frac;
> > +
> > +            /* If there are other outputs assigned to the same
> > location
> > +             * they must have the same interpolation
> > +             */
> > +            unsigned comp = 0;
> > +            while (comp < i) {
> > +               ir_variable *tmp = explicit_locations[idx][comp];
> > +               if (tmp && tmp->data.interpolation != var-
> > >data.interpolation) {
> > +                  linker_error(prog,
> > +                               "%s shader has multiple outputs at
> > explicit "
> > +                               "location %u with different
> > interpolation "
> > +                               "settings\n",
> > +                               _mesa_shader_stage_to_string(produc
> > er->Stage),
> > +                               idx);
> > +                  return;
> > +               }
> > +               comp++;
> > +            }
> > +
> > +            comp = last_comp + 1;

I just noticed that this should be:

comp = last_comp;

since that is the first component not used by this variable.

> > +            while (comp < 4) {
> > +               ir_variable *tmp = explicit_locations[idx][comp];
> > +               if (tmp && tmp->data.interpolation != var-
> > >data.interpolation) {
> > +                  linker_error(prog,
> > +                               "%s shader has multiple outputs at
> > explicit "
> > +                               "location %u with different
> > interpolation "
> > +                               "settings\n",
> > +                               _mesa_shader_stage_to_string(produc
> > er->Stage),
> > +                               idx);
> > +                  return;
> > +               }
> > +               comp++;
> > +            }
> 
> Can't we just put this check in the loop below? It should allow
> doubles 
> to be handled without the duplication if my quick skim over the code
> is 
> correct.

Mmm...I don't think so. The loop below checks the components used by
the current variable to see if there is aliasing in the range used by
the variable: [i, last_comp).  We need to check if there are other
variables assigned to any other components in the same location to
check if there is a different interpolation for them, so that means
that we need to check all the components in the location that are _not_
used by this variable, so basically, all the components not covered by
the loop below. This means the regions [0...i) and [last_comp...4].

We could make the loop below go up to 4 instead of last_comp and the
nhave ifs inside to do one thing in the range [i...last_comp) and
another one in the range [last_comp, 4] but that would only make things
more confusing IMHO, so I'd rather not do that.

Iago

> 
> > +
> > +            /* Component aliasing is not allowed */
> >               while (i < last_comp) {
> >                  if (explicit_locations[idx][i] != NULL) {
> >                     linker_error(prog,
> > 
> 
>