On Tue, 2015-01-20 at 12:34 +0100, Iago Toral wrote:
> On Mon, 2015-01-19 at 19:39 -0800, Ian Romanick wrote:
> > 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:
>
> Oh, this should not be here actually, it is a left over from my initial
> implementation and it is not really being used. Sorry about that, I'll
> remove it from the patch.
>
>
> > 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;
> > > };
>
> With the above said, this precision field that I am adding here goes
> into glsl_struct_field, which is part of the fields union in a glsl_type
> and that makes two struct definitions with different precision
> qualifiers for any of their fields be different types. However, I think
> that should be okay, I think we only compare fields of two structs
> (glsl_type::record_compare) to check if they are the same type, and we
> only use this from the linker to ensure that two definitions of the same
> global have the exact same type, which is precisely what we want to do
> here, since GLSL ES expects all declarations of the same uniform to have
> the same type, including precision qualifiers.
>
> > > 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."
>
> Sure.
>
> > > + *
> > > + * 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.
I have thought about this again, although I declared this as unsigned,
we only need it to store one of 4 precision enum values (highp, mediump,
lowp, none). Since this is inside the data struct, we can make it a bit
field so it only uses 2-bits, and lower the storage requirements to a
byte type too. We would still be increasing the size of ir_variable, but
the additional storage required would be much lower than 4-bytes. Does
this help?
Also, the stream field in ir_variable is a similar case. Since the
number of streams allowed by an implementation is not fixed we may not
want to turn that into a bitfield, but I guess we could at least lower
storage requirements from unsigned to a byte type. If this makes sense I
can send a patch.
> There is an alternative, although I am not a fan of it: instead of
> attaching a precision to every variable directly in the IR we can keep a
> separate map of mediump/lowp variables in the compiler (if a valid
> variable is not in the map, then it is highp). That should keep the
> additional footprint low and still provide what we need to add the link
> time checks, but I dislike that we would be moving part of the variable
> information to a separate place outside the IR. Also, I am not sure that
> this would be sufficient by the time we want to put precision qualifiers
> into actual use in a driver, I guess we will need the precision info to
> be available in the IR to do that.
>
> what do you think?
>
> > > +
> > > + /**
> > > * 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
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
