[Mesa-dev] [PATCH 05/13] glsl: simplifies the merge of the default in layout qualifier

[Timothy Arceri]

On Mon, 2016-11-21 at 18:08 +0200, Andres Gomez wrote:
> On Thu, 2016-11-17 at 16:17 +1100, Timothy Arceri wrote:
> > 
> > On Mon, 2016-11-14 at 19:15 +0200, Andres Gomez wrote:
> ...
> > 
> > > 
> > > diff --git a/src/compiler/glsl/ast_type.cpp
> > > b/src/compiler/glsl/ast_type.cpp
> > > index 803d952..064c63b 100644
> > > --- a/src/compiler/glsl/ast_type.cpp
> > > +++ b/src/compiler/glsl/ast_type.cpp
> ...
> > 
> > > 
> > > @@ -534,6 +549,45 @@
> > > ast_type_qualifier::validate_in_qualifier(YYLTYPE *loc,
> > >        _mesa_glsl_error(loc, state, "invalid input layout
> > > qualifiers
> > > used");
> > >     }
> > >  
> > > +   /* The checks below are also performed when merging but we
> > > want
> > > to spit an
> > > +    * error against the default global input qualifier as soon
> > > as we
> > > can, with
> > > +    * the closest error location in the shader.
> > > +    */
> > 
> > 
> > This comment looks like it should be removed? Don't the below
> > changes
> > remove the validation from the merge?
> 
> I don't think I follow what you mean.
> 
> The comment applies in the sense that the validation below is done
> against the default in qualifier while the validation done in the
> merge
> is between the 2 layout-qualifiers merging.
> 
> That validation of the merge will happen against the default in
> qualifier in any case, in the last step of the merging. However,
> since
> we want to fail ASAP and with the proper failing column, we have to
> validate explicitly against the default in qualifier every time after
> a
> merge.
> 
> In my opinion, the comment stands, although maybe I should rephrase.
> Dunno.

Ok. I see now. I needed to take a look at the final output of the
series to see what was going on. You are just moving these checks in
this patch not removing duplicates.

Can I ask that you write a follow up patch for this series that creates
a helper function for each of these validations. For example:

static bool
validate_ordering(loc, state, qualifier, new_qualifier)
{
   if (qualifier->flags.q.ordering && new_qualifier->flags.q.ordering
       && qualifier->ordering != new_qualifier->ordering) {
      _mesa_glsl_error(loc, state,
                       "conflicting ordering specified");
   }

   return true;
}

In merge_qualifier() just put the call in the outer if. For example:

if (q.flags.q.ordering && validate_ordering(...))

This will be better IMO as the code currently exists early when there
is no reason we shouldn't continue on and check the other layout
qualifiers before reporting errors.

This patch is:

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

> 
> > 
> > 
> > 
> > > 
> > > +
> > > +   /* Input layout qualifiers can be specified multiple
> > > +    * times in separate declarations, as long as they match.
> > > +    */
> > > +   if (state->in_qualifier->flags.q.prim_type && this-
> > > > 
> > > > flags.q.prim_type
> > > 
> > > +       && state->in_qualifier->prim_type != this->prim_type) {
> > > +      r = false;
> > > +      _mesa_glsl_error(loc, state,
> > > +                       "conflicting input primitive %s
> > > specified",
> > > +                       state->stage == MESA_SHADER_GEOMETRY ?
> > > +                       "type" : "mode");
> > > +   }
> > > +
> > > +   if (state->in_qualifier->flags.q.vertex_spacing
> > > +       && this->flags.q.vertex_spacing
> > > +       && state->in_qualifier->vertex_spacing != this-
> > > > 
> > > > vertex_spacing) {
> > > 
> > > +      r = false;
> > > +      _mesa_glsl_error(loc, state,
> > > +                       "conflicting vertex spacing specified");
> > > +   }
> > > +
> > > +   if (state->in_qualifier->flags.q.ordering && this-
> > > > 
> > > > flags.q.ordering
> > > 
> > > +       && state->in_qualifier->ordering != this->ordering) {
> > > +      r = false;
> > > +      _mesa_glsl_error(loc, state,
> > > +                       "conflicting ordering specified");
> > > +   }
> > > +
> > > +   if (state->in_qualifier->flags.q.point_mode && this-
> > > > 
> > > > flags.q.point_mode
> > > 
> > > +       && state->in_qualifier->point_mode != this->point_mode) {
> > > +      r  = false;
> > 
> >            ^
> > There is an extra space here          
> 
> I'll correct this.
>