[Mesa-dev] [PATCH 08/10] glsl: parser changes for GL_ARB_explicit_uniform_location
Tapani
tapani.palli at intel.com
Wed May 21 07:43:06 PDT 2014
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.
>>>> + return false;
>>>> + }
>>>> +
>>>> + return true;
>>>> + }
>>>> +
>>>> bool has_explicit_attrib_location() const
>>>> {
>>>> return ARB_explicit_attrib_location_enable || is_version(330,
>>>> 300);
>>>>
More information about the mesa-dev
mailing list