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

[Tapani Pälli]

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.

// Tapani