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

[Timothy Arceri]

On Sat, 2015-11-07 at 23:16 -0800, Jordan Justen wrote:
> 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.

Yeah your right, in that case feel free not to add this check. 

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