[Mesa-dev] [PATCH/RFC] glsl: allow redeclaring variables as 'precise invariant'

[Erik Faye-Lund]

Ping?

On on., aug. 22, 2018 at 7:34 PM, Erik Faye-Lund 
<erik.faye-lund at collabora.com> wrote:
> There's seems to be nothing in the GLSL (ES) specifications that 
> diallow
> redeclaring a variable as both 'precise' and 'invariant' in the same
> statement. But the way the parse-rules are structured this fails to
> parse, because this is handled in single_declaration, which has an
> exhaustive list of qualifiers here, and it does not include an option
> with both.
> 
> So let's factor out the precise/invariant handling from the
> type_qualifier rule so we can reuse it, as there's some intricate
> subtleties here.
> 
> For reference: glslangValidator already allows this.
> 
> Signed-off-by: Erik Faye-Lund <erik.faye-lund at collabora.com>
> ---
> 
> My yacc/bison skills aren't all that, so I wouldn't be too surprised 
> if
> this the wrong approach. This is why I'm posting this as an RFC. I'm
> hoping someone can tell me if this is the right kind of approach or 
> not.
> 
> This fixes a failure to compile a shader that appears in virgl after
> an invariant output has been lowered to precise TGSI-opcodes, and
> re-emitted GLSL for it (in virglrenderer). The problematic shader 
> comes from
> dEQP-GLES2.functional.shaders.invariance.highp.subexpression_precision_lowp.
> 
>  src/compiler/glsl/glsl_parser.yy | 77 
> +++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 41 deletions(-)
> 
> diff --git a/src/compiler/glsl/glsl_parser.yy 
> b/src/compiler/glsl/glsl_parser.yy
> index cb7376995d..042e654a26 100644
> --- a/src/compiler/glsl/glsl_parser.yy
> +++ b/src/compiler/glsl/glsl_parser.yy
> @@ -187,6 +187,7 @@ static bool match_layout_qualifier(const char 
> *s1, const char *s2,
>  %type <node> statement_list
>  %type <node> simple_statement
>  %type <n> precision_qualifier
> +%type <type_qualifier> invariant_or_precise_qualifier
>  %type <type_qualifier> type_qualifier
>  %type <type_qualifier> auxiliary_storage_qualifier
>  %type <type_qualifier> storage_qualifier
> @@ -1106,7 +1107,7 @@ single_declaration:
>        $$->declarations.push_tail(&decl->link);
>        state->symbols->add_variable(new(state) ir_variable(NULL, $2, 
> ir_var_auto));
>     }
> -   | INVARIANT variable_identifier
> +   | invariant_or_precise_qualifier variable_identifier
>     {
>        void *ctx = state->linalloc;
>        ast_declaration *decl = new(ctx) ast_declaration($2, NULL, 
> NULL);
> @@ -1114,19 +1115,8 @@ single_declaration:
> 
>        $$ = new(ctx) ast_declarator_list(NULL);
>        $$->set_location_range(@1, @2);
> -      $$->invariant = true;
> -
> -      $$->declarations.push_tail(&decl->link);
> -   }
> -   | PRECISE variable_identifier
> -   {
> -      void *ctx = state->linalloc;
> -      ast_declaration *decl = new(ctx) ast_declaration($2, NULL, 
> NULL);
> -      decl->set_location(@2);
> -
> -      $$ = new(ctx) ast_declarator_list(NULL);
> -      $$->set_location_range(@1, @2);
> -      $$->precise = true;
> +      $$->invariant = $1.flags.q.invariant;
> +      $$->precise = $1.flags.q.precise;
> 
>        $$->declarations.push_tail(&decl->link);
>     }
> @@ -1889,7 +1879,7 @@ interpolation_qualifier:
>     }
>     ;
> 
> -type_qualifier:
> +invariant_or_precise_qualifier:
>     /* Single qualifiers */
>     INVARIANT
>     {
> @@ -1901,31 +1891,7 @@ type_qualifier:
>        memset(& $$, 0, sizeof($$));
>        $$.flags.q.precise = 1;
>     }
> -   | auxiliary_storage_qualifier
> -   | storage_qualifier
> -   | interpolation_qualifier
> -   | layout_qualifier
> -   | memory_qualifier
> -   | subroutine_qualifier
> -   | precision_qualifier
> -   {
> -      memset(&$$, 0, sizeof($$));
> -      $$.precision = $1;
> -   }
> -
> -   /* Multiple qualifiers:
> -    * In GLSL 4.20, these can be specified in any order.  In earlier 
> versions,
> -    * they appear in this order (see GLSL 1.50 section 4.7 & 
> comments below):
> -    *
> -    *    invariant interpolation auxiliary storage precision  
> ...or...
> -    *    layout storage precision
> -    *
> -    * Each qualifier's rule ensures that the accumulated qualifiers 
> on the right
> -    * side don't contain any that must appear on the left hand side.
> -    * For example, when processing a storage qualifier, we check 
> that there are
> -    * no auxiliary, interpolation, layout, invariant, or precise 
> qualifiers to the right.
> -    */
> -   | PRECISE type_qualifier
> +   | PRECISE invariant_or_precise_qualifier
>     {
>        if ($2.flags.q.precise)
>           _mesa_glsl_error(&@1, state, "duplicate \"precise\" 
> qualifier");
> @@ -1933,7 +1899,7 @@ type_qualifier:
>        $$ = $2;
>        $$.flags.q.precise = 1;
>     }
> -   | INVARIANT type_qualifier
> +   | INVARIANT invariant_or_precise_qualifier
>     {
>        if ($2.flags.q.invariant)
>           _mesa_glsl_error(&@1, state, "duplicate \"invariant\" 
> qualifier");
> @@ -1958,6 +1924,35 @@ type_qualifier:
>        if (state->is_version(430, 300) && $$.flags.q.in)
>           _mesa_glsl_error(&@1, state, "invariant qualifiers cannot 
> be used with shader inputs");
>     }
> +
> +type_qualifier:
> +   /* Single qualifiers */
> +   invariant_or_precise_qualifier
> +   | auxiliary_storage_qualifier
> +   | storage_qualifier
> +   | interpolation_qualifier
> +   | layout_qualifier
> +   | memory_qualifier
> +   | subroutine_qualifier
> +   | precision_qualifier
> +   {
> +      memset(&$$, 0, sizeof($$));
> +      $$.precision = $1;
> +   }
> +
> +   /* Multiple qualifiers:
> +    * In GLSL 4.20, these can be specified in any order.  In earlier 
> versions,
> +    * they appear in this order (see GLSL 1.50 section 4.7 & 
> comments below):
> +    *
> +    *    invariant interpolation auxiliary storage precision  
> ...or...
> +    *    layout storage precision
> +    *
> +    * Each qualifier's rule ensures that the accumulated qualifiers 
> on the right
> +    * side don't contain any that must appear on the left hand side.
> +    * For example, when processing a storage qualifier, we check 
> that there are
> +    * no auxiliary, interpolation, layout, invariant, or precise 
> qualifiers to the right.
> +    */
> +
>     | interpolation_qualifier type_qualifier
>     {
>        /* Section 4.3 of the GLSL 1.40 specification states:
> --
> 2.17.1
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180829/69bba023/attachment.html>