[Mesa-dev] [PATCH 08/10] glsl: parser changes for GL_ARB_explicit_uniform_location
Tapani
tapani.palli at intel.com
Wed May 21 08:11:17 PDT 2014
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:
>>>>> 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 | 37
>>>>> +++++++++++++++++++++++++++++++++++++
>>>>> src/glsl/glcpp/glcpp-parse.y | 3 +++
>>>>> src/glsl/glsl_lexer.ll | 1 +
>>>>> src/glsl/glsl_parser_extras.h | 14 ++++++++++++++
>>>>> 4 files changed, 55 insertions(+)
>>>>>
>>>>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>>>>> index 8d55ee3..7431ad7 100644
>>>>> --- a/src/glsl/ast_to_hir.cpp
>>>>> +++ b/src/glsl/ast_to_hir.cpp
>>>>> @@ -2170,6 +2170,43 @@ validate_explicit_location(const struct
>>>>> ast_type_qualifier *qual,
>>>>> {
>>>>> bool fail = false;
>>>>> + /* Checks for GL_ARB_explicit_uniform_location. */
>>>>> + if (qual->flags.q.uniform) {
>>>>> +
>>>> Extra blank line.
>>> oops
>>>
>>>>> + if (!state->check_explicit_uniform_location_allowed(loc, var))
>>>>> + return;
>>>>> +
>>>>> + const struct gl_context *const ctx = state->ctx;
>>>>> + unsigned max_loc = qual->location +
>>>>> var->type->component_slots() - 1;
>>>> I think that over counts for this purpose, and we can blame confusing
>>>> nomenclature. component_slots for a mat4 is 4, so a mat4 uniform
>>>> counts
>>>> 4*4 against the GL_MAX_VERTEX_UNIFORM_COMPONENTS limit. However, it
>>>> only has one "location" (as returned by glGetUniformLocation), so it
>>>> only counts 1 against the GL_MAX_UNIFORM_LOCATIONS limit.
>>> I see, I was considering structs and arrays when writing this part and
>>> forgot about matrix. I assume matrix is the only special case here
>>> though? Everything else gets correct location count value via
>>> component_slots().
>> Currently, it should only bee matrices and structures containing
>> matrices
>> that need different treatment. I think dvec3 and dvec4 (when we add
>> support
>> for doubles) will also take multiple slots.
>
> Ok, I'll add additional helper to glsl_type for this as it is needed
> when assigning locations too.
>
>>>>> +
>>>>> + /* 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 qualifier for
>>>>> uniform %s "
>>>>> + ">= MAX_UNIFORM_LOCATIONS (%u)",
>>>>> + var->name,
>>>>> +
>>>>> ctx->Const.MaxUserAssignableUniformLocations);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + var->data.explicit_location = true;
>>>>> + var->data.location = qual->location;
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> /* Between GL_ARB_explicit_attrib_location an
>>>>> * GL_ARB_separate_shader_objects, the inputs and outputs of
>>>>> any
>>>>> shader
>>>>> * stage can be assigned explicit locations. The checking here
>>>>> associates
>>>>> diff --git a/src/glsl/glcpp/glcpp-parse.y
>>>>> b/src/glsl/glcpp/glcpp-parse.y
>>>>> index f28d853..6d42138 100644
>>>>> --- a/src/glsl/glcpp/glcpp-parse.y
>>>>> +++ b/src/glsl/glcpp/glcpp-parse.y
>>>>> @@ -2087,6 +2087,9 @@
>>>>> _glcpp_parser_handle_version_declaration(glcpp_parser_t *parser,
>>>>> intmax_t versio
>>>>> if (extensions->ARB_explicit_attrib_location)
>>>>> add_builtin_define(parser,
>>>>> "GL_ARB_explicit_attrib_location", 1);
>>>>> + if (extensions->ARB_explicit_uniform_location)
>>>>> + add_builtin_define(parser,
>>>>> "GL_ARB_explicit_uniform_location", 1);
>>>>> +
>>>>> if (extensions->ARB_shader_texture_lod)
>>>>> add_builtin_define(parser,
>>>>> "GL_ARB_shader_texture_lod", 1);
>>>>> diff --git a/src/glsl/glsl_lexer.ll b/src/glsl/glsl_lexer.ll
>>>>> index 7602351..83f0b6d 100644
>>>>> --- a/src/glsl/glsl_lexer.ll
>>>>> +++ b/src/glsl/glsl_lexer.ll
>>>>> @@ -393,6 +393,7 @@ layout {
>>>>> || yyextra->AMD_conservative_depth_enable
>>>>> || yyextra->ARB_conservative_depth_enable
>>>>> || yyextra->ARB_explicit_attrib_location_enable
>>>>> + || yyextra->ARB_explicit_uniform_location_enable
>>>>> || yyextra->has_separate_shader_objects()
>>>>> || yyextra->ARB_uniform_buffer_object_enable
>>>>> || yyextra->ARB_fragment_coord_conventions_enable
>>>>> 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.
>>>>> + return false;
>>>>> + }
>>>>> +
>>>>> + return true;
>>>>> + }
>>>>> +
>>>>> bool has_explicit_attrib_location() const
>>>>> {
>>>>> return ARB_explicit_attrib_location_enable ||
>>>>> is_version(330,
>>>>> 300);
>>>>>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list