Re: [Mesa-dev] [PATCH 1/3] glsl: Parse shared keyword for compute shader variables

[Timothy Arceri]

On Fri, 2015-11-06 at 17:56 -0800, Jordan Justen wrote:
> Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com>
> ---
> 
> Notes:
>     git://people.freedesktop.org/~jljusten/mesa cs-parse-shared-vars-v1
>     http://patchwork.freedesktop.org/bundle/jljusten/cs-parse-shared-vars-v1
>     
>     With these environment overrides:
>     
>       export MESA_GL_VERSION_OVERRIDE=4.3
>       export MESA_GLSL_VERSION_OVERRIDE=430
>       export MESA_EXTENSION_OVERRIDE=GL_ARB_compute_shader
>     
>     This fixes my recently posted piglit test:
>     
>       tests/spec/arb_compute_shader/compiler/shared-variables.comp
>       http://patchwork.freedesktop.org/patch/63944/
> 
>  src/glsl/ast_to_hir.cpp | 2 +-
>  src/glsl/glsl_lexer.ll  | 2 ++
>  src/glsl/glsl_parser.yy | 6 ++++++
>  3 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 0306530..dd5ba4e 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -3081,7 +3081,7 @@ apply_type_qualifier_to_variable(const struct
> ast_type_qualifier *qual,
>     if (qual->flags.q.std140 ||
>         qual->flags.q.std430 ||
>         qual->flags.q.packed ||
> -       qual->flags.q.shared) {
> +       (qual->flags.q.shared && (state->stage != MESA_SHADER_COMPUTE))) {
>        _mesa_glsl_error(loc, state,
>                         "uniform and shader storage block layout qualifiers
> "
>                         "std140, std430, packed, and shared can only be "
> diff --git a/src/glsl/glsl_lexer.ll b/src/glsl/glsl_lexer.ll
> index 2142817..e59f93e 100644
> --- a/src/glsl/glsl_lexer.ll
> +++ b/src/glsl/glsl_lexer.ll
> @@ -414,6 +414,8 @@ writeonly      KEYWORD_WITH_ALT(420, 300, 420, 310,
> yyextra->ARB_shader_image_lo
>  
>  atomic_uint     KEYWORD_WITH_ALT(420, 300, 420, 310, yyextra
> ->ARB_shader_atomic_counters_enable, ATOMIC_UINT);
>  
> +shared          KEYWORD_WITH_ALT(430, 310, 430, 310, yyextra
> ->ARB_compute_shader_enable, SHARED);
> +
>  struct               return STRUCT;
>  void         return VOID_TOK;
>  
> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
> index 4636435..2598356 100644
> --- a/src/glsl/glsl_parser.yy
> +++ b/src/glsl/glsl_parser.yy
> @@ -165,6 +165,7 @@ static bool match_layout_qualifier(const char *s1, const
> char *s2,
>  %token IMAGE1DSHADOW IMAGE2DSHADOW IMAGE1DARRAYSHADOW IMAGE2DARRAYSHADOW
>  %token COHERENT VOLATILE RESTRICT READONLY WRITEONLY
>  %token ATOMIC_UINT
> +%token SHARED
>  %token STRUCT VOID_TOK WHILE
>  %token <identifier> IDENTIFIER TYPE_IDENTIFIER NEW_IDENTIFIER
>  %type <identifier> any_identifier
> @@ -1958,6 +1959,11 @@ memory_qualifier:
>        memset(& $$, 0, sizeof($$));
>        $$.flags.q.write_only = 1;
>     }
> +   | SHARED
> +   {
> +      memset(& $$, 0, sizeof($$));
> +      $$.flags.q.shared = 1;
> +   }

Hi Jordan,

This should be in storage_qualifier: rather than memory_qualifier:

Also it should be restricted to the computer shader stage e.g.

if (state->stage == MESA_SHADER_COMPUTE) {
      memset(& $$, 0, sizeof($$));
      $$.flags.q.shared = 1;
} else {
        _mesa_glsl_error(&@1, state, "the shared storage qualifiers can only  
                         "be used with compute shaders");
}

Maybe add a piglit test to make sure it fails in another stage?

>     ;
>  
>  array_specifier:
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev