[Mesa-dev] [PATCH v2 2/3] glsl: allow built-in variables to be explicitly declared

[Ian Romanick]

On 05/11/2018 09:49 PM, Timothy Arceri wrote:
> Mesa seems to be the only implementation that doesn't allow builtins
> to be explicitly declared. The GLSL 1.30 spec seems to imply that
> buitins may be explicitly declared.

There was a clarification added to the spec in response to a Khronos bug
that I submitted.  I don't recall the version where it was added, but
the resulting text is in section 3.7 (Identifiers):

    Identifiers starting with “gl_” are reserved for use by OpenGL, and
    may not be declared in a shader; this results in a compile-time
    error. However, as noted in the specification, there are some cases
    where previously declared variables can be redeclared, and
    predeclared "gl_" names are allowed to be redeclared in a shader
   only for these specific purposes. More generally, it is a
   compile-time error to redeclare a variable, including those starting
   “gl_”.

The specific cases where redeclaration is allowed are cases where
invariant is added, precision qualifiers are added, an array size is
added, etc.

> This this allows the game "Full Bore" the be playable (when using
> MESA_GL_VERSION_OVERRIDE=3.3COMPAT). It will also allow us to
> remove the allow_glsl_builtin_variable_redeclaration dri override.

What is the nature of the redeclaration in this game?  Does the shader
in question compile with glslang?

> From the GLSL 1.30 spec Section 7.2 (Fragment Shader Special
> Variables):
> 
>     "Both gl_FragColor and gl_FragData are deprecated; the preferred
>     usage is to explicitly declare these outputs in the fragment
>     shader using the out storage qualifier."
> 
> To avoid some GLSL ES tests failing we add a check to make sure
> precision matches on the redeclared builtin.
> ---
>  src/compiler/glsl/ast_to_hir.cpp | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> index a7a9ac80769..54d0816a986 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -4390,14 +4390,8 @@ get_variable_being_redeclared(ir_variable **var_ptr, YYLTYPE loc,
>        earlier->data.precision = var->data.precision;
>        earlier->data.memory_coherent = var->data.memory_coherent;
>  
> -   } else if (earlier->data.how_declared == ir_var_declared_implicitly &&
> -              state->allow_builtin_variable_redeclaration) {
> -      /* Allow verbatim redeclarations of built-in variables. Not explicitly
> -       * valid, but some applications do it.
> -       */
> -      if (earlier->data.mode != var->data.mode &&
> -          !(earlier->data.mode == ir_var_system_value &&
> -            var->data.mode == ir_var_shader_in)) {
> +   } else if (allow_all_redeclarations) {
> +      if (earlier->data.mode != var->data.mode) {
>           _mesa_glsl_error(&loc, state,
>                            "redeclaration of `%s' with incorrect qualifiers",
>                            var->name);
> @@ -4406,8 +4400,22 @@ get_variable_being_redeclared(ir_variable **var_ptr, YYLTYPE loc,
>                            "redeclaration of `%s' has incorrect type",
>                            var->name);
>        }
> -   } else if (allow_all_redeclarations) {
> -      if (earlier->data.mode != var->data.mode) {
> +   } else if (earlier->data.how_declared == ir_var_declared_implicitly) {
> +      /* Allow verbatim redeclarations of built-in variables. The GLSL 1.30
> +       * spec seems to imply that buitins may be explicitly declared.
> +       *
> +       * From the GLSL 1.30 spec Section 7.2 (Fragment Shader Special
> +       * Variables):
> +       *
> +       *    "Both gl_FragColor and gl_FragData are deprecated; the preferred
> +       *    usage is to explicitly declare these outputs in the fragment
> +       *    shader using the out storage qualifier."
> +       */
> +      enum ir_variable_mode builtin_mode =
> +         glsl_external_mode((ir_variable_mode) earlier->data.mode,
> +                            state->stage, earlier->data.location);
> +
> +      if (builtin_mode != var->data.mode) {
>           _mesa_glsl_error(&loc, state,
>                            "redeclaration of `%s' with incorrect qualifiers",
>                            var->name);
> @@ -4415,6 +4423,10 @@ get_variable_being_redeclared(ir_variable **var_ptr, YYLTYPE loc,
>           _mesa_glsl_error(&loc, state,
>                            "redeclaration of `%s' has incorrect type",
>                            var->name);
> +      } else if (earlier->data.precision != var->data.precision) {
> +         _mesa_glsl_error(&loc, state,
> +                          "redeclaration of `%s' has incorrect precision",
> +                          var->name);
>        }
>     } else {
>        _mesa_glsl_error(&loc, state, "`%s' redeclared", var->name);
>