[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