[Mesa-dev] [PATCH] glsl: fix subroutine mismatch between declarations/definitions
Samuel Pitoiset
samuel.pitoiset at gmail.com
Thu Mar 2 18:58:25 UTC 2017
On 03/02/2017 07:56 PM, Ilia Mirkin wrote:
> Perhaps you ran the gpu profile?
Nope, tests/all.gpu
>
> On Thu, Mar 2, 2017 at 1:50 PM, Samuel Pitoiset
> <samuel.pitoiset at gmail.com> wrote:
>> I wonder why piglit didn't report me that failure because I ran it with -t
>> subroutine before sending the patch...
>>
>>
>> On 03/02/2017 06:56 PM, Mark Janes wrote:
>>>
>>> This patch regresses:
>>> piglit.spec.arb_shader_subroutine.linker.no-overloads_vert
>>>
>>> The program in the test should fail to link, but does not.
>>>
>>>
>>>
>>> Samuel Pitoiset <samuel.pitoiset at gmail.com> writes:
>>>
>>>> Previously, when q.subroutine was set to 1, a new subroutine
>>>> declaration was added to the AST, while 0 meant a subroutine
>>>> definition has been detected by the parser.
>>>>
>>>> Thus, setting the q.subroutine flag in both situations is
>>>> obviously wrong because a new type identifier is added instead
>>>> of trying to match the declaration. To fix it up, introduce
>>>> ast_type_qualifier::is_subroutine_decl() to differentiate
>>>> declarations and definitions easily.
>>>>
>>>> This fixes a regression with:
>>>> arb_shader_subroutine/compiler/direct-call.vert
>>>>
>>>> Cc: Mark Janes <mark.a.janes at intel.com>
>>>> Fixes: be8aa76afd ("glsl: remove unecessary flags.q.subroutine_def")
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100026
>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>>>> ---
>>>> src/compiler/glsl/ast.h | 5 +++++
>>>> src/compiler/glsl/ast_to_hir.cpp | 12 ++++++------
>>>> src/compiler/glsl/ast_type.cpp | 5 +++++
>>>> src/compiler/glsl/glsl_parser.yy | 2 +-
>>>> src/compiler/glsl/glsl_parser_extras.cpp | 2 +-
>>>> 5 files changed, 18 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
>>>> index d27b940744..55cc5df8f3 100644
>>>> --- a/src/compiler/glsl/ast.h
>>>> +++ b/src/compiler/glsl/ast.h
>>>> @@ -770,6 +770,11 @@ struct ast_type_qualifier {
>>>> */
>>>> bool has_memory() const;
>>>>
>>>> + /**
>>>> + * Return true if the qualifier is a subroutine declaration.
>>>> + */
>>>> + bool is_subroutine_decl() const;
>>>> +
>>>> bool merge_qualifier(YYLTYPE *loc,
>>>> _mesa_glsl_parse_state *state,
>>>> const ast_type_qualifier &q,
>>>> diff --git a/src/compiler/glsl/ast_to_hir.cpp
>>>> b/src/compiler/glsl/ast_to_hir.cpp
>>>> index a90813033f..59d03c9c96 100644
>>>> --- a/src/compiler/glsl/ast_to_hir.cpp
>>>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>>>> @@ -3256,7 +3256,7 @@ apply_explicit_location(const struct
>>>> ast_type_qualifier *qual,
>>>> }
>>>>
>>>> /* Check if index was set for the uniform instead of the function
>>>> */
>>>> - if (qual->flags.q.explicit_index && qual->flags.q.subroutine) {
>>>> + if (qual->flags.q.explicit_index && qual->is_subroutine_decl()) {
>>>> _mesa_glsl_error(loc, state, "an index qualifier can only be "
>>>> "used with subroutine functions");
>>>> return;
>>>> @@ -3742,7 +3742,7 @@ apply_type_qualifier_to_variable(const struct
>>>> ast_type_qualifier *qual,
>>>> }
>>>> }
>>>>
>>>> - if (qual->flags.q.subroutine && !qual->flags.q.uniform) {
>>>> + if (qual->is_subroutine_decl() && !qual->flags.q.uniform) {
>>>> _mesa_glsl_error(loc, state,
>>>> "`subroutine' may only be applied to uniforms, "
>>>> "subroutine type declarations, or function
>>>> definitions");
>>>> @@ -4780,7 +4780,7 @@ ast_declarator_list::hir(exec_list *instructions,
>>>> continue;
>>>> }
>>>>
>>>> - if (this->type->qualifier.flags.q.subroutine) {
>>>> + if (this->type->qualifier.is_subroutine_decl()) {
>>>> const glsl_type *t;
>>>> const char *name;
>>>>
>>>> @@ -4879,7 +4879,7 @@ ast_declarator_list::hir(exec_list *instructions,
>>>> */
>>>> if (this->type->qualifier.flags.q.attribute) {
>>>> mode = "attribute";
>>>> - } else if (this->type->qualifier.flags.q.subroutine) {
>>>> + } else if (this->type->qualifier.is_subroutine_decl()) {
>>>> mode = "subroutine uniform";
>>>> } else if (this->type->qualifier.flags.q.uniform) {
>>>> mode = "uniform";
>>>> @@ -5629,7 +5629,7 @@ ast_function::hir(exec_list *instructions,
>>>> f = state->symbols->get_function(name);
>>>> if (f == NULL) {
>>>> f = new(ctx) ir_function(name);
>>>> - if (!this->return_type->qualifier.flags.q.subroutine) {
>>>> + if (!this->return_type->qualifier.is_subroutine_decl()) {
>>>> if (!state->symbols->add_function(f)) {
>>>> /* This function name shadows a non-function use of the same
>>>> name. */
>>>> YYLTYPE loc = this->get_location();
>>>> @@ -5787,7 +5787,7 @@ ast_function::hir(exec_list *instructions,
>>>>
>>>> }
>>>>
>>>> - if (this->return_type->qualifier.flags.q.subroutine) {
>>>> + if (this->return_type->qualifier.is_subroutine_decl()) {
>>>> if (!state->symbols->add_type(this->identifier,
>>>> glsl_type::get_subroutine_instance(this->identifier))) {
>>>> _mesa_glsl_error(& loc, state, "type '%s' previously defined",
>>>> this->identifier);
>>>> return NULL;
>>>> diff --git a/src/compiler/glsl/ast_type.cpp
>>>> b/src/compiler/glsl/ast_type.cpp
>>>> index 5f868a81f2..d302fc45fd 100644
>>>> --- a/src/compiler/glsl/ast_type.cpp
>>>> +++ b/src/compiler/glsl/ast_type.cpp
>>>> @@ -112,6 +112,11 @@ bool ast_type_qualifier::has_memory() const
>>>> || this->flags.q.write_only;
>>>> }
>>>>
>>>> +bool ast_type_qualifier::is_subroutine_decl() const
>>>> +{
>>>> + return this->flags.q.subroutine && !this->subroutine_list;
>>>> +}
>>>> +
>>>> static bool
>>>> validate_prim_type(YYLTYPE *loc,
>>>> _mesa_glsl_parse_state *state,
>>>> diff --git a/src/compiler/glsl/glsl_parser.yy
>>>> b/src/compiler/glsl/glsl_parser.yy
>>>> index 59453d72f6..e703073c9c 100644
>>>> --- a/src/compiler/glsl/glsl_parser.yy
>>>> +++ b/src/compiler/glsl/glsl_parser.yy
>>>> @@ -900,7 +900,7 @@ function_header:
>>>> $$->return_type = $1;
>>>> $$->identifier = $2;
>>>>
>>>> - if ($1->qualifier.flags.q.subroutine) {
>>>> + if ($1->qualifier.is_subroutine_decl()) {
>>>> /* add type for IDENTIFIER search */
>>>> state->symbols->add_type($2,
>>>> glsl_type::get_subroutine_instance($2));
>>>> } else
>>>> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
>>>> b/src/compiler/glsl/glsl_parser_extras.cpp
>>>> index e88dd071b3..44fb46ab83 100644
>>>> --- a/src/compiler/glsl/glsl_parser_extras.cpp
>>>> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
>>>> @@ -1072,7 +1072,7 @@ _mesa_ast_process_interface_block(YYLTYPE *locp,
>>>> void
>>>> _mesa_ast_type_qualifier_print(const struct ast_type_qualifier *q)
>>>> {
>>>> - if (q->flags.q.subroutine)
>>>> + if (q->is_subroutine_decl())
>>>> printf("subroutine ");
>>>>
>>>> if (q->subroutine_list) {
>>>> --
>>>> 2.12.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list