On Wed, 2015-02-25 at 10:01 +0100, Iago Toral wrote:
> 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.
Actually, this part is not necessary, since the symbol table already
supports creating entries from ast_type_specifier.
> 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);
> > }
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
