[Mesa-dev] [PATCH 02/11] glsl: Add link time checks for GLSL precision qualifiers
Ian Romanick
idr at freedesktop.org
Fri Feb 6 00:47:47 PST 2015
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 <itoral at 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?
> 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);
>>>> }
More information about the mesa-dev
mailing list