[Mesa-dev] [PATCH] glsl: remove unecessary flags.q.subroutine_def

[Samuel Pitoiset]

On 03/01/2017 12:04 AM, Timothy Arceri wrote:
> On 01/03/17 03:57, Samuel Pitoiset wrote:
>>
>>
>> On 02/26/2017 10:19 PM, Timothy Arceri wrote:
>>>
>>>
>>> On 25/02/17 22:15, Samuel Pitoiset wrote:
>>>> This bit is definitely not necessary because subroutine_list
>>>> can be used instead. This frees one more bit in the flags.q
>>>> struct which is nice because arb_bindless_texture will need
>>>> 4 bits for the new layout qualifiers.
>>>>
>>>> No piglit regressions found (including compiler tests) with
>>>> "-t subroutine".
>>>>
>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>>>> ---
>>>>  src/compiler/glsl/ast.h                  | 1 -
>>>>  src/compiler/glsl/ast_to_hir.cpp         | 6 +++---
>>>>  src/compiler/glsl/ast_type.cpp           | 6 ++----
>>>>  src/compiler/glsl/glsl_parser.yy         | 1 -
>>>>  src/compiler/glsl/glsl_parser_extras.cpp | 2 +-
>>>>  5 files changed, 6 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
>>>> index 11a092e41c..d27b940744 100644
>>>> --- a/src/compiler/glsl/ast.h
>>>> +++ b/src/compiler/glsl/ast.h
>>>> @@ -607,7 +607,6 @@ struct ast_type_qualifier {
>>>>           /** \name Qualifiers for GL_ARB_shader_subroutine */
>>>>       /** \{ */
>>>>           unsigned subroutine:1;  /**< Is this marked 'subroutine' */
>>>> -         unsigned subroutine_def:1; /**< Is this marked 'subroutine'
>>>> with a list of types */
>>>>       /** \} */
>>>>
>>>>           /** \name Qualifiers for GL_KHR_blend_equation_advanced */
>>>> diff --git a/src/compiler/glsl/ast_to_hir.cpp
>>>> b/src/compiler/glsl/ast_to_hir.cpp
>>>> index f033d7df97..7e99faeaed 100644
>>>> --- a/src/compiler/glsl/ast_to_hir.cpp
>>>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>>>> @@ -3510,7 +3510,7 @@ apply_layout_qualifier_to_variable(const struct
>>>> ast_type_qualifier *qual,
>>>>           }
>>>>        }
>>>>     } else if (qual->flags.q.explicit_index) {
>>>> -      if (!qual->flags.q.subroutine_def)
>>>> +      if (!qual->subroutine_list)
>>>>           _mesa_glsl_error(loc, state,
>>>>                            "explicit index requires explicit
>>>> location");
>>>>     } else if (qual->flags.q.explicit_component) {
>>>> @@ -5568,7 +5568,7 @@ ast_function::hir(exec_list *instructions,
>>>>      *  "Subroutine declarations cannot be prototyped. It is an error
>>>> to prepend
>>>>      *   subroutine(...) to a function declaration."
>>>>      */
>>>> -   if (this->return_type->qualifier.flags.q.subroutine_def &&
>>>> !is_definition) {
>>>> +   if (this->return_type->qualifier.subroutine_list &&
>>>> !is_definition) {
>>>>        YYLTYPE loc = this->get_location();
>>>>        _mesa_glsl_error(&loc, state,
>>>>                         "function declaration `%s' cannot have
>>>> subroutine prepended",
>>>> @@ -5716,7 +5716,7 @@ ast_function::hir(exec_list *instructions,
>>>>     sig->replace_parameters(&hir_parameters);
>>>>     signature = sig;
>>>>
>>>> -   if (this->return_type->qualifier.flags.q.subroutine_def) {
>>>> +   if (this->return_type->qualifier.subroutine_list) {
>>>>        int idx;
>>>>
>>>>        if (this->return_type->qualifier.flags.q.explicit_index) {
>>>> diff --git a/src/compiler/glsl/ast_type.cpp
>>>> b/src/compiler/glsl/ast_type.cpp
>>>> index 96d20c10af..5f868a81f2 100644
>>>> --- a/src/compiler/glsl/ast_type.cpp
>>>> +++ b/src/compiler/glsl/ast_type.cpp
>>>> @@ -44,7 +44,6 @@
>>>> ast_fully_specified_type::has_qualifiers(_mesa_glsl_parse_state
>>>> *state) const
>>>>     ast_type_qualifier subroutine_only;
>>>>     subroutine_only.flags.i = 0;
>>>>     subroutine_only.flags.q.subroutine = 1;
>>>> -   subroutine_only.flags.q.subroutine_def = 1;
>>>>     if (state->has_explicit_uniform_location()) {
>>>>        subroutine_only.flags.q.explicit_index = 1;
>>>>     }
>>>> @@ -285,8 +284,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>>>>        }
>>>>     }
>>>>
>>>> -   if (q.flags.q.subroutine_def) {
>>>> -      if (this->flags.q.subroutine_def) {
>>>> +   if (q.subroutine_list) {
>>>> +      if (this->subroutine_list) {
>>>>           _mesa_glsl_error(loc, state,
>>>>                            "conflicting subroutine qualifiers used");
>>>>        } else {
>>>> @@ -772,7 +771,6 @@ ast_type_qualifier::validate_flags(YYLTYPE *loc,
>>>>                      bad.flags.q.point_mode ? " point_mode" : "",
>>>>                      bad.flags.q.vertices ? " vertices" : "",
>>>>                      bad.flags.q.subroutine ? " subroutine" : "",
>>>> -                    bad.flags.q.subroutine_def ? " subroutine_def" :
>>>> "",
>>>>                      bad.flags.q.blend_support ? " blend_support" : "",
>>>>                      bad.flags.q.inner_coverage ? " inner_coverage" :
>>>> "",
>>>>                      bad.flags.q.post_depth_coverage ? "
>>>> post_depth_coverage" : "");
>>>> diff --git a/src/compiler/glsl/glsl_parser.yy
>>>> b/src/compiler/glsl/glsl_parser.yy
>>>> index 7777d703f8..b79fcee550 100644
>>>> --- a/src/compiler/glsl/glsl_parser.yy
>>>> +++ b/src/compiler/glsl/glsl_parser.yy
>>>> @@ -1812,7 +1812,6 @@ subroutine_qualifier:
>>>>     | SUBROUTINE '(' subroutine_type_list ')'
>>>>     {
>>>>        memset(& $$, 0, sizeof($$));
>>>> -      $$.flags.q.subroutine_def = 1;
>>>
>>> You need to change this to:
>>>
>>>    $$.flags.q.subroutine = 1;
>>>
>>> Otherwise we won't detect if the qualifier was added when it should have
>>> been etc.
>>
>> Are you sure it's needed to set it up? I thought it was enough to only
>> set $$.subroutine_list.
>
> Sorry I should have been more clear. I mean the function that make use
> of checking the qualifier flags for validation won't work if you don't
> set something here. e.g. ast_type_qualifier::validate_flags

Right. Will send a v2.

>
>
>>
>>>
>>>
>>>>        $$.subroutine_list = $3;
>>>>     }
>>>>     ;
>>>> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
>>>> b/src/compiler/glsl/glsl_parser_extras.cpp
>>>> index 375a99a49d..e88dd071b3 100644
>>>> --- a/src/compiler/glsl/glsl_parser_extras.cpp
>>>> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
>>>> @@ -1075,7 +1075,7 @@ _mesa_ast_type_qualifier_print(const struct
>>>> ast_type_qualifier *q)
>>>>     if (q->flags.q.subroutine)
>>>>        printf("subroutine ");
>>>>
>>>> -   if (q->flags.q.subroutine_def) {
>>>> +   if (q->subroutine_list) {
>>>>        printf("subroutine (");
>>>>        q->subroutine_list->print();
>>>>        printf(")");
>>>>