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

[Tapani Pälli]

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.

> 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) {