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

[Ilia Mirkin]

On Wed, Aug 24, 2016 at 6:19 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 08/24/2016 12:57 PM, Ilia Mirkin wrote:
>> On Wed, Aug 24, 2016 at 3:40 PM, Ian Romanick <idr at freedesktop.org> wrote:
>>> On 08/24/2016 10:40 AM, Ilia Mirkin 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.
>>>>
>>>>  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),
>>>
>>> I think this will mean that having
>>>
>>> #extension GL_ARB_compute_shader: require
>>>
>>> will not compile.  That seems bad.
>>
>> I see no indication that that should ever have worked in the first place.
>
> There are a good number of conformance tests that do this.  That's
> making me start to question the validity of this change at all.

OK. Cheerfully withdrawn.

  -ilia