On Fri, 2015-02-06 at 10:47 +0200, Ian Romanick wrote:
> On 01/26/2015 10:09 AM, Iago Toral wrote:
> > 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?
>
> I was talking with Topi about this. We want to generate better code on
> some platforms for GLSL ES shaders that use mediump or lowp, so we will
> need this information (for all variables) soon anyway. ir_variable is,
> alas, the most sensible place to put it. It should be possible to use
> pahole to find a place to add 2 bits that won't increase the overall
> size of the object. Can you do that?
Sure, I'll give it a try.
Iago
> > 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
