[Mesa-dev] [PATCH] glsl: remove excess location qualifier validation
Tapani Pälli
tapani.palli at intel.com
Thu Oct 22 22:01:11 PDT 2015
On 10/22/2015 11:07 AM, Timothy Arceri wrote:
> 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);
> }
Yep, this is the case. Builtin variables have explicit locations but
those are set directly to the ir_variable when creating them. I can't
find any way of them being negative here (especially as parser just hits
syntax error with both attributes and uniforms for locations < 0):
Reviewed-by: Tapani Pälli <tapani.palli at intel.com>
>>> 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