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

[Iago Toral]

On Thu, 2017-10-19 at 08:29 +0200, Iago Toral wrote:
> 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_interp
> > > ol
> > > 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(prod
> > > uc
> > > 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(prod
> > > uc
> > > 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.

However, we can do a single loop and skip the components in [1,
last_comp) for these checks so we only have one loop instead of two.
I'll send a v2 with this change. I have another patch to add also
checks for the auxiliary storage and this way we don't have to
duplicate those too.

Iago

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