[Mesa-dev] [PATCH 08/10] glsl: parser changes for GL_ARB_explicit_uniform_location

[Ian Romanick]

On 05/22/2014 09:37 PM, Tapani Pälli wrote:
> On 05/21/2014 07:52 PM, Ian Romanick wrote:
>> On 05/21/2014 08:11 AM, Tapani wrote:
>>> On 05/21/2014 05:43 PM, Tapani wrote:
>>>> On 05/21/2014 01:27 AM, Ian Romanick wrote:
>>>>> On 05/19/2014 10:08 PM, Tapani wrote:
>>>>>> On 05/19/2014 08:18 PM, Ian Romanick wrote:
>>>>>>> On 04/09/2014 02:56 AM, Tapani Pälli wrote:
>>>>>>>> diff --git a/src/glsl/glsl_parser_extras.h
>>>>>>>> b/src/glsl/glsl_parser_extras.h
>>>>>>>> index c53c583..20879a0 100644
>>>>>>>> --- a/src/glsl/glsl_parser_extras.h
>>>>>>>> +++ b/src/glsl/glsl_parser_extras.h
>>>>>>>> @@ -152,6 +152,20 @@ struct _mesa_glsl_parse_state {
>>>>>>>>          return true;
>>>>>>>>       }
>>>>>>>>    +   bool check_explicit_uniform_location_allowed(YYLTYPE *locp,
>>>>>>>> +                                                const ir_variable
>>>>>>>> *var)
>>>>>>>> +   {
>>>>>>>> +      /* Requires OpenGL 3.3 or ARB_explicit_attrib_location. */
>>>>>>>> +      if (ctx->Version < 33 &&
>>>>>>>> !ctx->Extensions.ARB_explicit_attrib_location) {
>>>>>>>> +         _mesa_glsl_error(locp, this, "%s explicit location
>>>>>>>> requires "
>>>>>>>> + "GL_ARB_explicit_attrib_location extension "
>>>>>>>> +                          "or OpenGL 3.3", mode_string(var));
>>>>>>> Many copy-and-paste bugs. :) Explicit uniform locations aren't added
>>>>>>> until 4.3.
>>>>>> It may look copy-paste but the specification states that "Requires
>>>>>> OpenGL 3.3 or ARB_explicit_attrib_location":
>>>>>>
>>>>>> https://www.opengl.org/registry/specs/ARB/explicit_uniform_location.txt
>>>>>>
>>>>>> Using 4.3 capable driver will pass this check correctly.
>>>>> Oh right, because it relies on 3.3 or ARB_explicit_attib_location to
>>>>> add the layout keyword.  Some comments explaining that this isn't a
>>>>> copy-and-paste bug will prevent the next person from also thinking that
>>>>> it is. :)
>>>>>
>>>>> But this code should check the version (and extension) bits set in the
>>>>> shader, not what's enabled in the context. How about:
>>>>>
>>>>>     bool check_explicit_attrib_location_allowed(YYLTYPE *locp,
>>>>>                                                 const ir_variable *var)
>>>>>     {
>>>>>        if (!this->has_explicit_attrib_location() ||
>>>>>            !this->ARB_explicit_uniform_location_enable) {
>>>>>           _mesa_glsl_error(locp, this,
>>>>>                            "uniform explicit location requires
>>>>>                            "GL_ARB_explicit_uniform_location and
>>>>> either "
>>>>>                            "GL_ARB_explicit_attrib_location or GLSL
>>>>> 330.");
>>>>>           return false;
>>>>>        }
>>>>>
>>>>>        return true;
>>>>>     }
>>>> Sure, this is fine by me. I'll send new patches soon.
>>>>
>>> Or maybe fine with some changes since my piglit tests won't pass with
>>> this change (for those explicit attrib location is not available for
>>> some reason (!)), will take a look.
>> Do the tests enable it via #extension?
> 
> They enable GL_ARB_explicit_uniform_location but not
> GL_ARB_explicit_attrib_location and I think that is the way it should
> work. I don't understand why checking the existence of
> explicit_attrib_location from context is not correct way to deal with
> this? It doesn't need to be enabled in the language as layout token will
> be there also if just explicit_uniform_location is enabled.

There are two places that we need to check extension or version related
things in the compiler.

1. Check that the driver supports a particular extension when a shader
tries to enable the functionality (via #extension).  This is handled by
the _mesa_glsl_supported_extensions table in glsl_parser_extras.cpp.

2. Check that the shader has enabled the extension when it tries to use
some functionality from that extension.  This is handled by either
checking the appropriate state->foo_enable flag directly or using one of
the state->has_foo or state->check_foo methods.  The methods are used
for cases where a feature is enabled by multiple extensions or an
extension and a GLSL version.

Now, for this case...

I think the intention of the spec language is that the layout()
qualifier is added by either GL_ARB_explicit_attrib_location or GLSL
3.30.  The layout qualifier applied to a uniform is further added by
GL_ARB_explicit_uniform_location.

So... I think an application needs (GLSL 330 ||
GL_ARB_explicit_attrib_location) && GL_ARB_explicit_uniform_location.
Meaning that either

#version 330
#extension GL_ARB_explicit_uniform_location: enable

or

#version 120 // for example
#extension GL_ARB_explicit_attrib_location: enable
#extension GL_ARB_explicit_uniform_location: enable

I am curious to see what other implementatons do... if they do something
different from what I have said, I'll submit a spec bug so that it's
more clear.

> // Tapani