[Mesa-dev] [PATCH v3 04/14] glsl: process local_size_variable input qualifier

[Nicolai Hähnle]

On 26.09.2016 19:23, Samuel Pitoiset wrote:
> This is the new layout qualifier introduced by
> ARB_compute_variable_group_size which allows to use a variable work
> group size.
>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
>  src/compiler/glsl/ast.h                  |  5 +++++
>  src/compiler/glsl/ast_type.cpp           |  6 ++++++
>  src/compiler/glsl/glsl_parser.yy         | 13 +++++++++++++
>  src/compiler/glsl/glsl_parser_extras.cpp |  6 ++++++
>  src/compiler/glsl/glsl_parser_extras.h   |  6 ++++++
>  5 files changed, 36 insertions(+)
>
> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
> index 4c648d0..55f009a 100644
> --- a/src/compiler/glsl/ast.h
> +++ b/src/compiler/glsl/ast.h
> @@ -553,6 +553,11 @@ struct ast_type_qualifier {
>            */
>           unsigned local_size:3;
>
> +	 /** \name Layout qualifiers for ARB_compute_variable_group_size. */
> +	 /** \{ */
> +	 unsigned local_size_variable:1;
> +	 /** \} */
> +
>  	 /** \name Layout and memory qualifiers for ARB_shader_image_load_store. */
>  	 /** \{ */
>  	 unsigned early_fragment_tests:1;
> diff --git a/src/compiler/glsl/ast_type.cpp b/src/compiler/glsl/ast_type.cpp
> index f3f6b29..3f19f1f 100644
> --- a/src/compiler/glsl/ast_type.cpp
> +++ b/src/compiler/glsl/ast_type.cpp
> @@ -503,6 +503,7 @@ ast_type_qualifier::merge_in_qualifier(YYLTYPE *loc,
>           state->in_qualifier->flags.q.local_size == 0;
>
>        valid_in_mask.flags.q.local_size = 7;
> +      valid_in_mask.flags.q.local_size_variable = 1;
>        break;
>     default:
>        _mesa_glsl_error(loc, state,
> @@ -580,6 +581,10 @@ ast_type_qualifier::merge_in_qualifier(YYLTYPE *loc,
>        this->point_mode = q.point_mode;
>     }
>
> +   if (q.flags.q.local_size_variable) {
> +      state->cs_input_local_size_variable_specified = true;
> +   }

What if previously a fixed local group size was specified? I think you 
need to check for this either here or in the next patch.


> +
>     if (create_node) {
>        if (create_gs_ast) {
>           node = new(mem_ctx) ast_gs_input_layout(*loc, q.prim_type);
> @@ -653,6 +658,7 @@ ast_type_qualifier::validate_flags(YYLTYPE *loc,
>                      bad.flags.q.prim_type ? " prim_type" : "",
>                      bad.flags.q.max_vertices ? " max_vertices" : "",
>                      bad.flags.q.local_size ? " local_size" : "",
> +                    bad.flags.q.local_size_variable ? " local_size_variable" : "",
>                      bad.flags.q.early_fragment_tests ? " early_fragment_tests" : "",
>                      bad.flags.q.explicit_image_format ? " image_format" : "",
>                      bad.flags.q.coherent ? " coherent" : "",

You need to add a %s to the monster format string above. Doesn't this 
trigger a compiler warning?

Cheers,
Nicolai


> diff --git a/src/compiler/glsl/glsl_parser.yy b/src/compiler/glsl/glsl_parser.yy
> index 9e1fd9e..38cbd3f 100644
> --- a/src/compiler/glsl/glsl_parser.yy
> +++ b/src/compiler/glsl/glsl_parser.yy
> @@ -1491,6 +1491,19 @@ layout_qualifier_id:
>           }
>        }
>
> +      /* Layout qualifiers for ARB_compute_variable_group_size. */
> +      if (!$$.flags.i) {
> +         if (match_layout_qualifier($1, "local_size_variable", state) == 0) {
> +            $$.flags.q.local_size_variable = 1;
> +         }
> +
> +         if ($$.flags.i && !state->ARB_compute_variable_group_size_enable) {
> +            _mesa_glsl_error(& @1, state,
> +                             "qualifier `local_size_variable` requires "
> +                             "ARB_compute_variable_group_size");
> +         }
> +      }
> +
>        if (!$$.flags.i) {
>           _mesa_glsl_error(& @1, state, "unrecognized layout identifier "
>                            "`%s'", $1);
> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp b/src/compiler/glsl/glsl_parser_extras.cpp
> index eff5235..e200b7d 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -297,6 +297,8 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
>            sizeof(this->atomic_counter_offsets));
>     this->allow_extension_directive_midshader =
>        ctx->Const.AllowGLSLExtensionDirectiveMidShader;
> +
> +   this->cs_input_local_size_variable_specified = false;
>  }
>
>  /**
> @@ -1676,6 +1678,7 @@ set_shader_inout_layout(struct gl_shader *shader,
>     if (shader->Stage != MESA_SHADER_COMPUTE) {
>        /* Should have been prevented by the parser. */
>        assert(!state->cs_input_local_size_specified);
> +      assert(!state->cs_input_local_size_variable_specified);
>     }
>
>     if (shader->Stage != MESA_SHADER_FRAGMENT) {
> @@ -1791,6 +1794,9 @@ set_shader_inout_layout(struct gl_shader *shader,
>           for (int i = 0; i < 3; i++)
>              shader->info.Comp.LocalSize[i] = 0;
>        }
> +
> +      shader->info.Comp.LocalSizeVariable =
> +         state->cs_input_local_size_variable_specified;
>        break;
>
>     case MESA_SHADER_FRAGMENT:
> diff --git a/src/compiler/glsl/glsl_parser_extras.h b/src/compiler/glsl/glsl_parser_extras.h
> index 7528df7..127edbc 100644
> --- a/src/compiler/glsl/glsl_parser_extras.h
> +++ b/src/compiler/glsl/glsl_parser_extras.h
> @@ -405,6 +405,12 @@ struct _mesa_glsl_parse_state {
>     unsigned cs_input_local_size[3];
>
>     /**
> +    * True if a compute shader input local variable size was specified using
> +    * a layout directive as specified by ARB_compute_variable_group_size.
> +    */
> +   bool cs_input_local_size_variable_specified;
> +
> +   /**
>      * Output layout qualifiers from GLSL 1.50 (geometry shader controls),
>      * and GLSL 4.00 (tessellation control shader).
>      */
>