[Mesa-dev] [PATCH] glsl: remove excess location qualifier validation

[Timothy Arceri]

On Thu, 2015-10-22 at 08:55 +0300, Tapani Pälli wrote:
> On 10/22/2015 08:29 AM, Timothy Arceri wrote:
> > Location has never been able to be a negative value because it has
> > always been validated in the parser.
> > 
> > Also the linker doesn't check for negatives like the comment
> > claims.
> 
> Neither does the parser, if one utilizes negative explicit location, 
> parser says:
> 
> error: syntax error, unexpected '-', expecting INTCONSTANT or
> UINTCONSTANT
> 
> I'm not sure if this is quite OK, it should rather accept the
> negative 
> value and the fail here in this check you are about to remove?

Yes I did noticed this. However even if we changed it to accept a
negative value the is another check in the parser that would catch this
before the checks I'm removing here.

if ($3 >= 0) {
   $$.location = $3;
} else {
    _mesa_glsl_error(& @3, state, "invalid location %d specified", $3);
             YYERROR;
}

I'm also working on ARB_enhanced_layouts which will allow negative
values to get passed the parser and will be moving the check out of the
parser and into the ast.

This patch is a clean-up before doing that, as the attributes code
doesn't do the validation at all and the one for uniforms should be
shared with the attibutes code.

> 
> > ---
> > 
> >   No piglit regressions and an extra negative test sent for
> >   ARB_explicit_uniform_location [1]
> > 
> >   [1] http://patchwork.freedesktop.org/patch/62573/
> > 
> >   src/glsl/ast_to_hir.cpp | 70 ++++++++++++++++--------------------
> > -------------
> >   1 file changed, 22 insertions(+), 48 deletions(-)
> > 
> > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> > index 8549d55..0306530 100644
> > --- a/src/glsl/ast_to_hir.cpp
> > +++ b/src/glsl/ast_to_hir.cpp
> > @@ -2422,21 +2422,6 @@ validate_explicit_location(const struct
> > ast_type_qualifier *qual,
> >         const struct gl_context *const ctx = state->ctx;
> >         unsigned max_loc = qual->location + var->type
> > ->uniform_locations() - 1;
> >   
> > -      /* ARB_explicit_uniform_location specification states:
> > -       *
> > -       *     "The explicitly defined locations and the generated
> > locations
> > -       *     must be in the range of 0 to MAX_UNIFORM_LOCATIONS
> > minus one."
> > -       *
> > -       *     "Valid locations for default-block uniform variable
> > locations
> > -       *     are in the range of 0 to the implementation-defined
> > maximum
> > -       *     number of uniform locations."
> > -       */
> > -      if (qual->location < 0) {
> > -         _mesa_glsl_error(loc, state,
> > -                          "explicit location < 0 for uniform %s",
> > var->name);
> > -         return;
> > -      }
> > -
> >         if (max_loc >= ctx
> > ->Const.MaxUserAssignableUniformLocations) {
> >            _mesa_glsl_error(loc, state, "location(s) consumed by
> > uniform %s "
> >                             ">= MAX_UNIFORM_LOCATIONS (%u)", var
> > ->name,
> > @@ -2527,41 +2512,30 @@ validate_explicit_location(const struct
> > ast_type_qualifier *qual,
> >      } else {
> >         var->data.explicit_location = true;
> >   
> > -      /* This bit of silliness is needed because invalid explicit
> > locations
> > -       * are supposed to be flagged during linking.  Small
> > negative values
> > -       * biased by VERT_ATTRIB_GENERIC0 or FRAG_RESULT_DATA0 could
> > alias
> > -       * built-in values (e.g., -16+VERT_ATTRIB_GENERIC0 =
> > VERT_ATTRIB_POS).
> > -       * The linker needs to be able to differentiate these cases.
> >   This
> > -       * ensures that negative values stay negative.
> > -       */
> > -      if (qual->location >= 0) {
> > -         switch (state->stage) {
> > -         case MESA_SHADER_VERTEX:
> > -            var->data.location = (var->data.mode ==
> > ir_var_shader_in)
> > -               ? (qual->location + VERT_ATTRIB_GENERIC0)
> > -               : (qual->location + VARYING_SLOT_VAR0);
> > -            break;
> > +      switch (state->stage) {
> > +      case MESA_SHADER_VERTEX:
> > +         var->data.location = (var->data.mode == ir_var_shader_in)
> > +            ? (qual->location + VERT_ATTRIB_GENERIC0)
> > +            : (qual->location + VARYING_SLOT_VAR0);
> > +         break;
> >   
> > -         case MESA_SHADER_TESS_CTRL:
> > -         case MESA_SHADER_TESS_EVAL:
> > -         case MESA_SHADER_GEOMETRY:
> > -            if (var->data.patch)
> > -               var->data.location = qual->location +
> > VARYING_SLOT_PATCH0;
> > -            else
> > -               var->data.location = qual->location +
> > VARYING_SLOT_VAR0;
> > -            break;
> > +      case MESA_SHADER_TESS_CTRL:
> > +      case MESA_SHADER_TESS_EVAL:
> > +      case MESA_SHADER_GEOMETRY:
> > +         if (var->data.patch)
> > +            var->data.location = qual->location +
> > VARYING_SLOT_PATCH0;
> > +         else
> > +            var->data.location = qual->location +
> > VARYING_SLOT_VAR0;
> > +         break;
> >   
> > -         case MESA_SHADER_FRAGMENT:
> > -            var->data.location = (var->data.mode ==
> > ir_var_shader_out)
> > -               ? (qual->location + FRAG_RESULT_DATA0)
> > -               : (qual->location + VARYING_SLOT_VAR0);
> > -            break;
> > -         case MESA_SHADER_COMPUTE:
> > -            assert(!"Unexpected shader type");
> > -            break;
> > -         }
> > -      } else {
> > -         var->data.location = qual->location;
> > +      case MESA_SHADER_FRAGMENT:
> > +         var->data.location = (var->data.mode ==
> > ir_var_shader_out)
> > +            ? (qual->location + FRAG_RESULT_DATA0)
> > +            : (qual->location + VARYING_SLOT_VAR0);
> > +         break;
> > +      case MESA_SHADER_COMPUTE:
> > +         assert(!"Unexpected shader type");
> > +         break;
> >         }
> >   
> >         if (qual->flags.q.explicit_index) {
>