[Mesa-dev] [WIP 11/13] glsl: parser changes for GL_ARB_explicit_uniform_location

[Ian Romanick]

On 04/06/2014 10:27 PM, Tapani Pälli wrote:
> On 04/05/2014 12:58 AM, Ian Romanick wrote:
>> On 04/04/2014 02:42 PM, Ian Romanick wrote:
>>> On 03/27/2014 11:45 PM, Tapani Pälli wrote:
>>>> Patch adds a preprocessor define for the extension and stores
>>>> explicit location data for uniforms during AST->HIR conversion.
>>>> It also sets layout token to be available when having the
>>>> extension in place.
>>>>
>>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>>> ---
>>>>  src/glsl/ast_to_hir.cpp      | 13 +++++++++++++
>>>>  src/glsl/glcpp/glcpp-parse.y |  3 +++
>>>>  src/glsl/glsl_lexer.ll       |  1 +
>>>>  3 files changed, 17 insertions(+)
>>>>
>>>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>>>> index 8f6e901..3bfad02 100644
>>>> --- a/src/glsl/ast_to_hir.cpp
>>>> +++ b/src/glsl/ast_to_hir.cpp
>>>> @@ -2123,6 +2123,19 @@ validate_explicit_location(const struct ast_type_qualifier *qual,
>>>>  {
>>>>     bool fail = false;
>>>>  
>>>> +   /* Checks for GL_ARB_explicit_uniform_location. */
>>>> +   if (qual->flags.q.uniform) {
>>>> +
>>>> +      if (qual->index < 0) {
>>>> +         _mesa_glsl_error(loc, state,
>>>> +                          "explicit location < 0 for uniform %s", var->name);
>>> We also need to check that the explicit location isn't too large.  I
>>> don't think that check can happen here because (I think) it will depend
>>> on the size of the uniform.  The spec isn't completely clear, but I think
>>>
>>> // assume GL_MAX_UNIFORM_LOCATIONS is 65536
>>> layout(location=65535) float foo[1];
>>>
>>> should fail because foo[1] would have location 65536 (larger than
>>> GL_MAX_UNIFORM_LOCATIONS-1).
>>>
>>> Given the other rules in the spec about counting and use of locations, I
>>> think ast_to_hir is the right time to do the upper bound check.  I
>>> believe we can figure out how many locations a structure or array
>>> uniform will maximally use at that point.
>> I now see that you added that check in patch 8.  Maybe just put a
>> comment here explaining why the check can't happen here.
> 
> OK, I'll try to move check for bounds to happen while parsing to be able
> to fail earlier. I originally preferred to have all checks in one place
> and location assignment felt like the right spot.

I think the check is fine where it is.  I just want a comment here so
that the next person doesn't say, "Hey!  We need to check the other
bound too!" :)