[Mesa-dev] [PATCH v2] glsl: Add link time checks for GLSL precision qualifiers

Iago Toral itoral at igalia.com
Wed Feb 25 01:01:21 PST 2015


On Fri, 2015-02-06 at 15:03 +0100, Iago Toral Quiroga wrote:
> Currently, we only consider precision qualifiers at compile-time. This patch
> adds precision information to ir_variable so we can also do link time checks.
> Specifically, from the GLSL ES3 spec, 4.5.3 Precision Qualifiers:
> 
> "The same uniform declared in different shaders that are linked together
>  must have the same precision qualification."
> 
> Notice that this patch will check the above also for GLSL ES globals that are
> not uniforms. This is not explicitly stated in the spec, but seems to be
> the only consistent choice: since we can only have one definition of a global
> all its declarations should be identical, including precision qualifiers.
> 
> v2 (Ian Romanick):
> - Remove unused precision field from glsl_type.
> - Fix comments stating that desktop OpenGL ignores precision qualifiers.
> - Make the precision field in ir_variable a bitfield and use pahole to
>   place it in an available hole so we don't increase memory storage
>   requirements for ir_variable.
> 
> No piglit regressions.
> 
> Fixes the following 4 dEQP tests:
> dEQP-GLES3.functional.shaders.linkage.uniform.struct.precision_conflict_1
> dEQP-GLES3.functional.shaders.linkage.uniform.struct.precision_conflict_2
> dEQP-GLES3.functional.shaders.linkage.uniform.struct.precision_conflict_3
> dEQP-GLES3.functional.shaders.linkage.uniform.struct.precision_conflict_4

I have just realized that this is not good enough since it does not
consider default precision qualifiers.

Unfortunately, the semantics for default precision qualifiers are not
implemented in Mesa either. As far as I can see, we only have the
minimum stuff we need to detect the error where don't declare a default
precision qualifier for float variables in a fragment shader and the
shader declares float variables without an explicit precision qualifier.
Even this implementation is rather hackish, since it creates a fake
"#default precision" variable in the symbol table to track if a default
precision for float variables has been set.

So I need to add an implementation for this too. The problem with
default precision qualifiers is that they have the same scoping rules as
variables, so I think we really want to use the symbol table to track
them and their values for each scope.

My proposal for this is:

1) Define a new kind of symbol (maybe something like
ast_default_(precision?)_qualifier. We want a new symbol because default
precision qualifiers are different from other other symbols we have and
they are not going to be used in the same way. I think the 'ast' prefix
makes sense here, since we only want to keep track of precision
qualifiers to properly setup ir_variables in the AST->IR process. There
won't be references to default precision qualifiers in the IR.

2) We populate the symbol table with default precision values as stated
by the spec as soon as the symbol table is created. This would be global
symbols.

3) When a default precision qualifier is processed in
ast_type_specifier::hir, we create the symbol and add it to the symbol
table. Since we have different precision qualifiers for different types,
we need to include a reference to the type in the name (so we can create
symbols such as #default_precision_float, #default_precision_int, etc)
The constructor for the symbol should also take the value
(GLSL_PRECISION_LOWP, etc) as parameter.

4) When it is time to assign a precision qualifier to a variable that
does not have an explicit precision qualifier, we lookup the
corresponding symbol considering the type of the variable. If it exists,
we take the precision qualifier to use from its value, otherwise we
generate a compilation error. 

I think this should cover all the semantics that we need here...

Iago

> ---
>  src/glsl/ast_to_hir.cpp | 12 ++++++++++++
>  src/glsl/glsl_types.cpp |  4 ++++
>  src/glsl/glsl_types.h   | 12 ++++++++++++
>  src/glsl/ir.h           | 13 +++++++++++++
>  src/glsl/linker.cpp     | 48 ++++++++++++++++++++++++++++++++++++++++--------
>  5 files changed, 81 insertions(+), 8 deletions(-)
> 
> According to pahole there was exactly a 2-bit hole after the index:1 bitfield:
> 
> unsigned int               from_named_ifc_block_nonarray:1; /*     0: 5  4 */
> unsigned int               from_named_ifc_block_array:1; /*     0: 4  4 */
> unsigned int               must_be_shader_input:1; /*     0: 3  4 */
> unsigned int               index:1;              /*     0: 2  4 */
> 
> /* XXX 2 bits hole, try to pack */
> 
> enum ir_depth_layout       depth_layout:3;       /*     4:29  4 */
> unsigned int               image_read_only:1;    /*     4:28  4 */
> 
> 
> Making precision a 2-bit bitfield right after index:1 fills the hole
> without adding any additional storage requirements:
> 
> unsigned int               from_named_ifc_block_nonarray:1; /*     0: 5  4 */
> unsigned int               from_named_ifc_block_array:1; /*     0: 4  4 */
> unsigned int               must_be_shader_input:1; /*     0: 3  4 */
> unsigned int               index:1;              /*     0: 2  4 */
> unsigned int               precision:2;          /*     0: 0  4 */
> enum ir_depth_layout       depth_layout:3;       /*     4:29  4 */
> unsigned int               image_read_only:1;    /*     4:28  4 */
> unsigned int               image_write_only:1;   /*     4:27  4 */
> 
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index ed0eb09..39957ad 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2458,6 +2458,10 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>     if (qual->flags.q.sample)
>        var->data.sample = 1;
>  
> +   /* Precision qualifiers do not hold any meaning in Desktop GLSL */
> +   if (state->es_shader && qual->precision)
> +      var->data.precision = qual->precision;
> +
>     if (state->stage == MESA_SHADER_GEOMETRY &&
>         qual->flags.q.out && qual->flags.q.stream) {
>        var->data.stream = qual->stream;
> @@ -5218,6 +5222,10 @@ ast_process_structure_or_interface_block(exec_list *instructions,
>           fields[i].centroid = qual->flags.q.centroid ? 1 : 0;
>           fields[i].sample = qual->flags.q.sample ? 1 : 0;
>  
> +         /* Precision qualifiers do not hold any meaning in Desktop GLSL */
> +         fields[i].precision = state->es_shader ? qual->precision :
> +                                                  GLSL_PRECISION_NONE;
> +
>           /* Only save explicitly defined streams in block's field */
>           fields[i].stream = qual->flags.q.explicit_stream ? qual->stream : -1;
>  
> @@ -5715,6 +5723,10 @@ ast_interface_block::hir(exec_list *instructions,
>           var->data.sample = fields[i].sample;
>           var->init_interface_type(block_type);
>  
> +         /* Precision qualifiers do not hold any meaning in Desktop GLSL */
> +         var->data.precision = state->es_shader ? fields[i].precision :
> +                                                  GLSL_PRECISION_NONE;
> +
>           if (fields[i].matrix_layout == GLSL_MATRIX_LAYOUT_INHERITED) {
>              var->data.matrix_layout = matrix_layout == GLSL_MATRIX_LAYOUT_INHERITED
>                 ? GLSL_MATRIX_LAYOUT_COLUMN_MAJOR : matrix_layout;
> diff --git a/src/glsl/glsl_types.cpp b/src/glsl/glsl_types.cpp
> index b4223f4..e35c2aa 100644
> --- a/src/glsl/glsl_types.cpp
> +++ b/src/glsl/glsl_types.cpp
> @@ -122,6 +122,7 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields,
>        this->fields.structure[i].centroid = fields[i].centroid;
>        this->fields.structure[i].sample = fields[i].sample;
>        this->fields.structure[i].matrix_layout = fields[i].matrix_layout;
> +      this->fields.structure[i].precision = fields[i].precision;
>     }
>  
>     mtx_unlock(&glsl_type::mutex);
> @@ -668,6 +669,9 @@ glsl_type::record_compare(const glsl_type *b) const
>        if (this->fields.structure[i].sample
>            != b->fields.structure[i].sample)
>           return false;
> +      if (this->fields.structure[i].precision
> +          != b->fields.structure[i].precision)
> +         return false;
>     }
>  
>     return true;
> diff --git a/src/glsl/glsl_types.h b/src/glsl/glsl_types.h
> index 441015c..d521f3d 100644
> --- a/src/glsl/glsl_types.h
> +++ b/src/glsl/glsl_types.h
> @@ -100,6 +100,13 @@ enum glsl_matrix_layout {
>     GLSL_MATRIX_LAYOUT_ROW_MAJOR
>  };
>  
> +enum {
> +   GLSL_PRECISION_NONE = 0,
> +   GLSL_PRECISION_HIGH,
> +   GLSL_PRECISION_MEDIUM,
> +   GLSL_PRECISION_LOW
> +};
> +
>  #ifdef __cplusplus
>  #include "GL/gl.h"
>  #include "util/ralloc.h"
> @@ -741,6 +748,11 @@ struct glsl_struct_field {
>      * streams (as in ir_variable::stream). -1 otherwise.
>      */
>     int stream;
> +
> +   /**
> +    * Precission qualifier
> +    */
> +   unsigned precision;
>  };
>  
>  static inline unsigned int
> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> index a0f48b2..9fe05ab 100644
> --- a/src/glsl/ir.h
> +++ b/src/glsl/ir.h
> @@ -740,6 +740,19 @@ public:
>        unsigned index:1;
>  
>        /**
> +       * Precision qualifier.
> +       *
> +       * In desktop GLSL we do not care about precision qualifiers at all, in
> +       * fact, the spec says that precision qualifiers are ignored.
> +       *
> +       * To make things easy, we make it so that this field is always
> +       * GLSL_PRECISION_NONE on desktop shaders. This way all the variables
> +       * have the same precision value and the checks we add in the compiler
> +       * for this field will never break a desktop shader compile.
> +       */
> +      unsigned precision:2;
> +
> +      /**
>         * \brief Layout qualifier for gl_FragDepth.
>         *
>         * This is not equal to \c ir_depth_layout_none if and only if this
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 3f5eac1..091f00f 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -756,14 +756,23 @@ cross_validate_globals(struct gl_shader_program *prog,
>  		   && existing->type->is_record()
>  		   && existing->type->record_compare(var->type)) {
>  		  existing->type = var->type;
> -	       } else {
> -		  linker_error(prog, "%s `%s' declared as type "
> -			       "`%s' and type `%s'\n",
> -			       mode_string(var),
> -			       var->name, var->type->name,
> -			       existing->type->name);
> -		  return;
> -	       }
> +         } else if (strcmp(var->type->name, existing->type->name)) {
> +            linker_error(prog, "%s `%s' declared as type "
> +                         "`%s' and type `%s'\n",
> +                         mode_string(var),
> +                         var->name, var->type->name,
> +                         existing->type->name);
> +            return;
> +         } else {
> +            /* The global is declared with the same type name but the type
> +             * declarations mismatch (e.g. the same struct type name, but
> +             * the actual struct declarations mismatch).
> +             */
> +            linker_error(prog, "%s `%s' declared with mismatching definitions "
> +                         "of type `%s'\n",
> +                         mode_string(var), var->name, var->type->name);
> +            return;
> +         }
>  	    }
>  
>  	    if (var->data.explicit_location) {
> @@ -918,6 +927,29 @@ cross_validate_globals(struct gl_shader_program *prog,
>                              mode_string(var), var->name);
>                 return;
>              }
> +            /* From the GLSL ES3 spec, 4.5.3 Precision qualifiers:
> +             *
> +             * "The same uniform declared in different shaders that are linked
> +             *  together must have the same precision qualification."
> +             *
> +             * In the GLSL ES2 spec this was resolved in the issue amendments
> +             * (10.3 Precision Qualifiers). The GLSL ES1 spec overlooked this,
> +             * but seems like an obvious error since we can only have one
> +             * consistent definition of a global.
> +             *
> +             * The desktop GLSL spec does not include this reference
> +             * because precision qualifiers are ignored. We will never
> +             * hit this scenario in desktop GLSL though because we always set
> +             * the precision of variables to GLSL_PRECISION_NONE.
> +             */
> +            if (var->data.mode == ir_var_uniform) {
> +               if (existing->data.precision != var->data.precision) {
> +                  linker_error(prog, "declarations for %s `%s` have "
> +                               "mismatching precision qualifiers\n",
> +                               mode_string(var), var->name);
> +                  return;
> +               }
> +            }
>  	 } else
>  	    variables.add_variable(var);
>        }




More information about the mesa-dev mailing list