[Mesa-dev] [PATCH 11/28] glsl: cross validate varyings with a component qualifier

[Timothy Arceri]

On Thu, 2016-01-07 at 13:47 -0800, Anuj Phogat wrote:
> On Mon, Dec 28, 2015 at 9:00 PM, Timothy Arceri
> <timothy.arceri at collabora.com> wrote:
> > This change checks for component overlap, including handling
> > overlap of
> > locations and components by doubles. Previously there was no
> > validation
> > for assigning explicit locations to a location used by the second
> > half
> > of a double.
> > 
> > V2: fix component matching for matricies
> > ---
> >  src/glsl/link_varyings.cpp | 71
> > +++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 60 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/glsl/link_varyings.cpp
> > b/src/glsl/link_varyings.cpp
> > index dea8741..496b987 100644
> > --- a/src/glsl/link_varyings.cpp
> > +++ b/src/glsl/link_varyings.cpp
> > @@ -222,7 +222,7 @@ cross_validate_outputs_to_inputs(struct
> > gl_shader_program *prog,
> >                                  gl_shader *producer, gl_shader
> > *consumer)
> >  {
> >     glsl_symbol_table parameters;
> > -   ir_variable *explicit_locations[MAX_VARYING] = { NULL, };
> > +   ir_variable *explicit_locations[MAX_VARYING][4] = { {NULL,
> > NULL} };
> > 
> >     /* Find all shader outputs in the "producer" stage.
> >      */
> > @@ -243,18 +243,67 @@ cross_validate_outputs_to_inputs(struct
> > gl_shader_program *prog,
> >           unsigned num_elements = type
> > ->count_attribute_slots(false);
> >           unsigned idx = var->data.location - VARYING_SLOT_VAR0;
> >           unsigned slot_limit = idx + num_elements;
> > +         unsigned last_comp;
> > +
> > +         if (var->type->without_array()->is_record()) {
> > +            /* The componet qualifier can't be used on structs so
> > just treat
> > +             * all component slots as used.
> > +             */
> > +            last_comp = 4;
> > +         } else {
> > +            last_comp = var->data.location_frac +
> > +               var->type->without_array()->vector_elements;
> > +         }
> > 
> >           while(idx < slot_limit) {
> > -            if (explicit_locations[idx] != NULL) {
> > -               linker_error(prog,
> > -                         "%s shader has multiple outputs
> > explicitly "
> > -                         "assigned to location %d\n",
> > -                         _mesa_shader_stage_to_string(producer
> > ->Stage),
> > -                         idx);
> > -               return;
> > -            }
> > +            bool d_second_location = false;
> > +            for (unsigned i = var->data.location_frac; i <
> > last_comp; i++) {
> > +               if (explicit_locations[idx][i] != NULL) {
> > +                  linker_error(prog,
> > +                               "%s shader has multiple outputs
> > explicitly "
> > +                               "assigned to location %d and
> > component %d\n",
> > +                              
> >  _mesa_shader_stage_to_string(producer->Stage),
> > +                               idx, var->data.location_frac);
> > +                  return;
> > +               }
> > 
> > -            explicit_locations[idx] = var;
> > +               /* Make sure all component at this location have
> > the same type.
> > +                */
> > +               for (unsigned j = 0; j < 4; j++) {
> > +                  if (explicit_locations[idx][j] &&
> > +                      (explicit_locations[idx][j]->type
> > ->without_array()
> > +                       ->base_type != var->type->without_array()
> > ->base_type)) {
> > +                     linker_error(prog,
> > +                                  "Varyings sharing the same
> > location must "
> > +                                  "have the same underlying
> > numerical type. "
> > +                                  "Location %u component %u\n",
> > idx,
> > +                                  var->data.location_frac);
> > +                     return;
> > +                  }
> > +               }
> > +
> > +               explicit_locations[idx][i] = var;
> With this code we end up setting only components 0 and 1 for dvec2.
> My
> understanding is dvec2 will take all 4 components?

Nice spotting, I've sent a new version of the patch which makes double
handling much nicer and fixes this issue.

There is also a new piglit to test this case:

http://patchwork.freedesktop.org/patch/69671/