[Mesa-dev] [PATCH] GLSL: Implementation of the GL_ARB_shading_language_420pack extension
Kenneth Graunke
kenneth at whitecape.org
Wed May 1 12:04:20 PDT 2013
On 04/21/2013 04:08 PM, Todd Previte wrote:
> Initial work on the implementation of GL_ARB_shading_language_420pack.
> The patch adds the functionality from this extension which allows
> for C-style array initialization for GLSL. The extension enable bits
> and extension definition are also included in this patch.
Todd,
Thanks for your work on this! A few high level comments:
It would be great to split this into a few patches:
1. mesa: Add extension support for ARB_shading_language_420pack.
This would be the extensions.c, mtypes.h, and glsl_parser_extras.*
changes that just add the ability to enable the extension.
2. glsl: Add support for C-style array initializers.
This would be the rest of this patch, and could be tested with
MESA_EXTENSION_OVERRIDE=GL_ARB_shading_language_420pack.
3. glsl: Add support for swizzles on scalars.
4. glsl: Add support for implicit conversions of return values.
...
n: mesa: Enable ARB_shading_language_420pack for 1.30+ drivers.
Which would finally enable & advertise the extension.
That way, each patch is a self-contained and testable feature.
I've got some stylistic nits below, but otherwise the code looks good.
Are there Piglit tests for this?
> ---
> src/glsl/ast.h | 13 ++++++++++-
> src/glsl/ast_function.cpp | 25 ++++++++++++++++++++++
> src/glsl/ast_to_hir.cpp | 5 +++++
> src/glsl/glsl_parser.yy | 45 ++++++++++++++++++++++++++++++++++++++-
> src/glsl/glsl_parser_extras.cpp | 1 +
> src/glsl/glsl_parser_extras.h | 2 ++
> src/mesa/main/extensions.c | 1 +
> src/mesa/main/mtypes.h | 1 +
> 8 files changed, 91 insertions(+), 2 deletions(-)
>
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> index 7300271..cf75250 100644
> --- a/src/glsl/ast.h
> +++ b/src/glsl/ast.h
> @@ -189,7 +189,8 @@ enum ast_operators {
> ast_float_constant,
> ast_bool_constant,
>
> - ast_sequence
> + ast_sequence,
> + ast_array_init
> };
>
> /**
> @@ -292,6 +293,16 @@ private:
> bool cons;
> };
>
> +/**
> + Array initialization class
> + */
> +class ast_array_initializer : public ast_expression {
> +public:
> + ast_array_initializer() : ast_expression(ast_array_init, NULL, NULL, NULL) { }
> + ast_type_specifier *constructor_type;
> + virtual ir_rvalue *hir(exec_list *instructions,
> + struct _mesa_glsl_parse_state *state);
> +};
>
> /**
> * Number of possible operators for an ast_expression
> diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
> index 26f72cf..0143204 100644
> --- a/src/glsl/ast_function.cpp
> +++ b/src/glsl/ast_function.cpp
> @@ -1511,3 +1511,28 @@ ast_function_expression::hir(exec_list *instructions,
>
> return ir_rvalue::error_value(ctx);
> }
> +
> +ir_rvalue *
> +ast_array_initializer::hir(exec_list *instructions, struct _mesa_glsl_parse_state *state)
> +{
> + YYLTYPE loc = this->get_location();
> + ir_rvalue *result = NULL;
> + const char *name;
> + const glsl_type *const constructor_type = this->constructor_type->glsl_type(&name, state);
> +
> + // Check here for correct extension and extension enable
For some reason, we try to avoid C++-style comments. Please use C-style.
> + if (state->ARB_shading_language_420pack_enable) {
> + result = process_array_constructor(instructions,
> + constructor_type,
> + &loc,
> + &this->expressions,
> + state);
Indentation is too big here. The correct style is 3-space, no tabs.
(Existing code does have tabs, but that was a mistake...new code should
be all spaces.)
> + }
> + else {
Please do "} else {". Some older core Mesa code uses this style, but
all the new stuff we've been doing cuddles both braces.
> + _mesa_glsl_error(&loc, state, "C-style array initialization requires " \
No need for the \ here.
> + "GL_ARB_shading_language_420pack extension\n");
> + }
> +
> + return result;
> +}
> +
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 2638411..44e289d 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -1026,6 +1026,11 @@ ast_expression::hir(exec_list *instructions,
> loc = this->get_location();
>
> switch (this->oper) {
> +
> + case ast_array_init:
> + assert(!"ast_array_init: Should never get here. Bad things have occurred.\n");
> + break;
> +
> case ast_assign: {
> op[0] = this->subexpressions[0]->hir(instructions, state);
> op[1] = this->subexpressions[1]->hir(instructions, state);
> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
> index f52ed9b..d042c14 100644
> --- a/src/glsl/glsl_parser.yy
> +++ b/src/glsl/glsl_parser.yy
> @@ -220,6 +220,7 @@ static void yyerror(YYLTYPE *loc, _mesa_glsl_parse_state *st, const char *msg)
> %type <declarator_list> init_declarator_list
> %type <declarator_list> single_declaration
> %type <expression> initializer
> +%type <expression> initializer-list
Could you please rename this to "initializer_list" (here and below)?
All the other production rules use underscores. I didn't realize that
using hyphens actually worked in bison :)
> %type <node> declaration
> %type <node> declaration_statement
> %type <node> jump_statement
> @@ -1021,6 +1022,13 @@ single_declaration:
> $$ = new(ctx) ast_declarator_list($1);
> $$->set_location(yylloc);
> $$->declarations.push_tail(&decl->link);
> + if ($6->oper == ast_array_init)
> + {
> + ast_array_initializer *ai = (ast_array_initializer*)$6;
> + ai->constructor_type = new(ctx) ast_type_specifier("");
> + *ai->constructor_type = *$1->specifier;
> + ai->constructor_type->is_array = true;
> + }
> }
> | fully_specified_type any_identifier '[' constant_expression ']' '=' initializer
> {
> @@ -1030,6 +1038,14 @@ single_declaration:
> $$ = new(ctx) ast_declarator_list($1);
> $$->set_location(yylloc);
> $$->declarations.push_tail(&decl->link);
> + if ($7->oper == ast_array_init)
> + {
> + ast_array_initializer *ai = (ast_array_initializer*)$7;
> + ai->constructor_type = new(ctx) ast_type_specifier("");
> + *ai->constructor_type = *$1->specifier;
> + ai->constructor_type->is_array = true;
> + ai->constructor_type->array_size = $4;
> + }
> }
> | fully_specified_type any_identifier '=' initializer
> {
> @@ -1039,6 +1055,11 @@ single_declaration:
> $$ = new(ctx) ast_declarator_list($1);
> $$->set_location(yylloc);
> $$->declarations.push_tail(&decl->link);
> + if ($4->oper == ast_array_init)
> + {
> + ast_array_initializer *ai = (ast_array_initializer*)$4;
> + ai->constructor_type = $1->specifier;
> + }
> }
> | INVARIANT variable_identifier // Vertex only.
> {
> @@ -1563,7 +1584,29 @@ struct_declarator:
>
> initializer:
> assignment_expression
> - ;
> + | '{' initializer-list '}'
> + {
> + $$ = $2;
> + }
> + | '{' initializer-list ',' '}'
> + {
> + $$ = $2;
> + }
> + ;
> +
> +initializer-list:
> + initializer
> + {
> + void *ctx = state;
> + $$ = new(ctx) ast_array_initializer();
> + $$->set_location(yylloc);
> + $$->expressions.push_tail(& $1->link);
> + }
> + | initializer-list ',' initializer
> + {
> + $1->expressions.push_tail(& $3->link);
> + }
> + ;
>
> declaration_statement:
> declaration
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> index 0992294..c954c07 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -466,6 +466,7 @@ static const _mesa_glsl_extension _mesa_glsl_supported_extensions[] = {
> EXT(OES_standard_derivatives, false, false, true, false, true, OES_standard_derivatives),
> EXT(ARB_texture_cube_map_array, true, false, true, true, false, ARB_texture_cube_map_array),
> EXT(ARB_shading_language_packing, true, false, true, true, false, ARB_shading_language_packing),
> + EXT(ARB_shading_language_420pack, true, true, true, true, false, ARB_shading_language_420pack),
> EXT(ARB_texture_multisample, true, false, true, true, false, ARB_texture_multisample),
> EXT(ARB_texture_query_lod, false, false, true, true, false, ARB_texture_query_lod),
> };
> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> index 95891b5..e64cee8 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -284,6 +284,8 @@ struct _mesa_glsl_parse_state {
> bool ARB_texture_multisample_warn;
> bool ARB_texture_query_lod_enable;
> bool ARB_texture_query_lod_warn;
> + bool ARB_shading_language_420pack_enable;
> + bool ARB_shading_language_420pack_warn;
> /*@}*/
>
> /** Extensions supported by the OpenGL implementation. */
> diff --git a/src/mesa/main/extensions.c b/src/mesa/main/extensions.c
> index c7f038b..4ac3c08 100644
> --- a/src/mesa/main/extensions.c
> +++ b/src/mesa/main/extensions.c
> @@ -126,6 +126,7 @@ static const struct extension extension_table[] = {
> { "GL_ARB_shader_texture_lod", o(ARB_shader_texture_lod), GL, 2009 },
> { "GL_ARB_shading_language_100", o(ARB_shading_language_100), GLL, 2003 },
> { "GL_ARB_shading_language_packing", o(ARB_shading_language_packing), GL, 2011 },
> + { "GL_ARB_shading_language_420pack", o(ARB_shading_language_420pack), GL, 2011 },
> { "GL_ARB_shadow", o(ARB_shadow), GLL, 2001 },
> { "GL_ARB_sync", o(ARB_sync), GL, 2003 },
> { "GL_ARB_texture_border_clamp", o(ARB_texture_border_clamp), GLL, 2000 },
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index e46fa39..5d9fe74 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -2982,6 +2982,7 @@ struct gl_extensions
> GLboolean ARB_shader_texture_lod;
> GLboolean ARB_shading_language_100;
> GLboolean ARB_shading_language_packing;
> + GLboolean ARB_shading_language_420pack;
> GLboolean ARB_shadow;
> GLboolean ARB_sync;
> GLboolean ARB_texture_border_clamp;
>
More information about the mesa-dev
mailing list