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

[Jordan Justen]

On 2015-11-06 19:33:38, Timothy Arceri wrote:
> On Fri, 2015-11-06 at 17:56 -0800, Jordan Justen wrote:
> > Signed-off-by: Jordan Justen <jordan.l.justen at 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?

I tested nvidia, and they don't fail to compile when a variable is
declared as shared in the render stages.

The spec says:

   "Variables declared as shared may only be used in compute shaders"

Unfortunately, that doesn't specifically say that it should fail at
the compile step.

I think it is fine for us to fail at the compile phase, but I'm not
sure about tests for this.

-Jordan