[Mesa-dev] [PATCH] glsl: remove ARB_compute_shader ext, always allow local_size_* qualifiers

[Ilia Mirkin]

On Wed, Aug 24, 2016 at 1:40 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> The GL_ARB_compute_shader spec does not make a mention of a #extension
> enable, nor a #define with the ext name. It follows that local_size_*
> should always be allowed in a compute shader stage, and the #extension
> support removed.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97447#c7
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> Cc: mesa-stable at lists.freedesktop.org
> ---
>
> The change to exposing the gl_MaxCompute* constants is a little dodgy, arguably
> it should be exposed everywhere when compute functionality is available, but
> that is a little annoying to do, and is no worse than the previous state of
> things.

Actually it's not so bad -- adding in something like


 --- a/src/compiler/glsl/builtin_variables.cpp
 +++ b/src/compiler/glsl/builtin_variables.cpp
@@ -789,7 +789,8 @@ builtin_variable_generator::generate_constants()
       }
    }

-   if (state->is_version(430, 310) || state->stage == MESA_SHADER_COMPUTE) {
+   if (state->is_version(430, 310) ||
+       (!state->es_shader && state->ctx->Extensions.ARB_compute_shader)) {
       add_const("gl_MaxComputeAtomicCounterBuffers",
                 state->Const.MaxComputeAtomicCounterBuffers);
       add_const("gl_MaxComputeAtomicCounters",

should be enough, I think. But it's unclear whether it's desired...

>
>  src/compiler/glsl/builtin_variables.cpp  | 2 +-
>  src/compiler/glsl/glsl_lexer.ll          | 4 ++--
>  src/compiler/glsl/glsl_parser.yy         | 5 ++---
>  src/compiler/glsl/glsl_parser_extras.cpp | 1 -
>  src/compiler/glsl/glsl_parser_extras.h   | 7 -------
>  5 files changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/src/compiler/glsl/builtin_variables.cpp b/src/compiler/glsl/builtin_variables.cpp
> index c9d8b1c..4f88170 100644
> --- a/src/compiler/glsl/builtin_variables.cpp
> +++ b/src/compiler/glsl/builtin_variables.cpp
> @@ -789,7 +789,7 @@ builtin_variable_generator::generate_constants()
>        }
>     }
>
> -   if (state->is_version(430, 310) || state->ARB_compute_shader_enable) {
> +   if (state->is_version(430, 310) || state->stage == MESA_SHADER_COMPUTE) {
>        add_const("gl_MaxComputeAtomicCounterBuffers",
>                  state->Const.MaxComputeAtomicCounterBuffers);
>        add_const("gl_MaxComputeAtomicCounters",
> diff --git a/src/compiler/glsl/glsl_lexer.ll b/src/compiler/glsl/glsl_lexer.ll
> index 7be2b89..3324cde 100644
> --- a/src/compiler/glsl/glsl_lexer.ll
> +++ b/src/compiler/glsl/glsl_lexer.ll
> @@ -410,7 +410,7 @@ 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);
> +shared          KEYWORD_WITH_ALT(430, 310, 430, 310, yyextra->stage == MESA_SHADER_COMPUTE, SHARED);
>
>  struct         return STRUCT;
>  void           return VOID_TOK;
> @@ -425,7 +425,7 @@ layout              {
>                       || yyextra->ARB_uniform_buffer_object_enable
>                       || yyextra->ARB_fragment_coord_conventions_enable
>                        || yyextra->ARB_shading_language_420pack_enable
> -                      || yyextra->ARB_compute_shader_enable
> +                      || yyextra->stage == MESA_SHADER_COMPUTE
>                        || yyextra->ARB_tessellation_shader_enable) {
>                       return LAYOUT_TOK;
>                    } else {
> diff --git a/src/compiler/glsl/glsl_parser.yy b/src/compiler/glsl/glsl_parser.yy
> index 4f4a83c..cf2fb60 100644
> --- a/src/compiler/glsl/glsl_parser.yy
> +++ b/src/compiler/glsl/glsl_parser.yy
> @@ -1570,10 +1570,9 @@ layout_qualifier_id:
>        for (int i = 0; i < 3; i++) {
>           if (match_layout_qualifier(local_size_qualifiers[i], $1,
>                                      state) == 0) {
> -            if (!state->has_compute_shader()) {
> +            if (state->stage != MESA_SHADER_COMPUTE) {
>                 _mesa_glsl_error(& @3, state,
> -                                "%s qualifier requires GLSL 4.30 or "
> -                                "GLSL ES 3.10 or ARB_compute_shader",
> +                                "%s qualifier only valid in a compute shader",
>                                  local_size_qualifiers[i]);
>                 YYERROR;
>              } else {
> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp b/src/compiler/glsl/glsl_parser_extras.cpp
> index 14a5540..37aff38 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -578,7 +578,6 @@ static const _mesa_glsl_extension _mesa_glsl_supported_extensions[] = {
>     EXT(ARB_ES3_1_compatibility),
>     EXT(ARB_ES3_2_compatibility),
>     EXT(ARB_arrays_of_arrays),
> -   EXT(ARB_compute_shader),
>     EXT(ARB_conservative_depth),
>     EXT(ARB_cull_distance),
>     EXT(ARB_derivative_control),
> diff --git a/src/compiler/glsl/glsl_parser_extras.h b/src/compiler/glsl/glsl_parser_extras.h
> index 0294ef7..e536606 100644
> --- a/src/compiler/glsl/glsl_parser_extras.h
> +++ b/src/compiler/glsl/glsl_parser_extras.h
> @@ -260,11 +260,6 @@ struct _mesa_glsl_parse_state {
>        return ARB_shading_language_420pack_enable || is_version(420, 310);
>     }
>
> -   bool has_compute_shader() const
> -   {
> -      return ARB_compute_shader_enable || is_version(430, 310);
> -   }
> -
>     bool has_shader_io_blocks() const
>     {
>        /* The OES_geometry_shader_specification says:
> @@ -557,8 +552,6 @@ struct _mesa_glsl_parse_state {
>     bool ARB_ES3_2_compatibility_warn;
>     bool ARB_arrays_of_arrays_enable;
>     bool ARB_arrays_of_arrays_warn;
> -   bool ARB_compute_shader_enable;
> -   bool ARB_compute_shader_warn;
>     bool ARB_conservative_depth_enable;
>     bool ARB_conservative_depth_warn;
>     bool ARB_cull_distance_enable;
> --
> 2.7.3
>