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

[Timothy Arceri]

On 23/05/18 03:16, Ian Romanick wrote:
> 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?

It explicitly declares the builtin as it is specified in the spec:

#version 130
out vec4 gl_FragColor;

I'm assuming it was done to get rid of warnings that the builtin has 
been depreciated. This seems to be inline with the spec quote:

 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."

>  Does the shader
> in question compile with glslang?

No it doesn't. However its a little sad that the spec was updated to 
specifically disallow explicit declaration of builtins but we have 0 
piglit tests and 0 CTS tests for it. Especially since every other 
implementation allows it.


> 
>>  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);
>>
>