[Mesa-dev] [PATCH] glsl: remove excess location qualifier validation
Timothy Arceri
timothy.arceri at collabora.com
Thu Oct 22 01:07:07 PDT 2015
On Thu, 2015-10-22 at 09:56 +0300, Tapani Pälli wrote:
> On 10/22/2015 09:41 AM, Timothy Arceri wrote:
> > 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;
> > }
>
> OK, then removing the check for uniform loc should be fine. For the
> attributes change, I'm not sure yet, I believe the reason for the
> 'silliness' (mentioned in the commit) is that all the built-in
> attributes have negative locations, we need to be careful to continue
> dealing gracefully with that.
This function is only valdating the qualifier, builtins don't seem to
set the qualifier flags in the ast, as far as I can tell they set the
fields in the ir directly.
For location the only place that sets the explicit location flag is the
parser and this validation function is only called when that is set, so
I don't believe this should be a problem.
if (qual->flags.q.explicit_location) {
validate_explicit_location(qual, var, state, loc);
}
>
> > 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) {
>
More information about the mesa-dev
mailing list