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

[Tapani Pälli]

On 05/28/2014 09:41 PM, Ian Romanick wrote:
> 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

This is how Nvidia binary driver works, it requires '#version 330'.

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

I don't understand why user needs to worry about dependencies between
extensions here but maybe it's just me and I'm ok to change the check to
this approach, this is how it would work with the proposed changes applied.

> 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

// Tapani