[Mesa-dev] [PATCH] glsl: fix subroutine mismatch between declarations/definitions
Ilia Mirkin
imirkin at alum.mit.edu
Thu Mar 2 18:56:33 UTC 2017
Perhaps you ran the gpu profile?
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