On 01/19/2015 03:32 AM, Eduardo Lima Mitev wrote:
> From: Iago Toral Quiroga <ito...@igalia.com>
>
> 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.
Probably not. In ES you can only have one compilation unit per stage,
so you can never have such a conflict.
> 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
> ---
> src/glsl/ast_to_hir.cpp | 12 ++++++++++++
> src/glsl/glsl_types.cpp | 4 ++++
> src/glsl/glsl_types.h | 13 +++++++++++++
> src/glsl/ir.h | 15 +++++++++++++++
> src/glsl/linker.cpp | 48 ++++++++++++++++++++++++++++++++++++++++--------
> 5 files changed, 84 insertions(+), 8 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 811a955..2b67e14 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2460,6 +2460,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;
> @@ -5213,6 +5217,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;
>
> @@ -5688,6 +5696,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..2405006 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"
> @@ -117,6 +124,7 @@ struct glsl_type {
> * and \c GLSL_TYPE_UINT are valid.
> */
> unsigned interface_packing:2;
> + unsigned precision:2;
Eh... putting precision in the GLSL type doesn't seem right. It seems
that will cause problems with type checking things like:
lowp float x = 1.0;
highp float y = x;
>
> /* Callers of this ralloc-based new need not call delete. It's
> * easier to just ralloc_free 'mem_ctx' (or any of its ancestors). */
> @@ -741,6 +749,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..b17c244 100644
> --- a/src/glsl/ir.h
> +++ b/src/glsl/ir.h
> @@ -825,6 +825,21 @@ public:
> unsigned max_array_access;
>
> /**
> + * Precision qualifier.
> + *
> + * In desktop GLSL we do not care about precision qualifiers at all, in
> + * fact the spec does not care about some incoherent uses of qualifiers
> + * that do raise an error in GLSL ES (specifically, declaring the same
> + * uniform in multiple shaders with different precision qualifiers).
Instead, I would say, "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;
I'm not very comfortable putting this in ir_variable. This class is one
of the main sources of memory (over-)usage in the compiler. Making
every ir_variable 4 bytes bigger is pretty painful.
Unless we can come up with a different way, I think I'm going to have to
make subclassing ir_variable work. This hasn't already been done
because we don't know the kind of ir_variable (e.g., local, uniform,
shader input, etc.) until after we allocate the object and add it to the
symbol table.
> +
> + /**
> * Allow (only) ir_variable direct access private members.
> */
> friend class ir_variable;
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 3f5eac1..485b7e1 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,
> probably
> + * because it basically ignores precision qualifiers. We will
> never
* 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);
> }
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
