On Tue, 2015-11-10 at 12:41 +0200, Tapani Pälli wrote:
>
> On 11/10/2015 12:26 PM, Iago Toral wrote:
> > On Fri, 2015-11-06 at 14:03 +0200, Tapani Pälli wrote:
> >> From: Iago Toral Quiroga <ito...@igalia.com>
> >>
> >> We will need this later on when we implement proper support for
> >> precision qualifiers in the drivers and also to do link time checks for
> >> uniforms as indicated by the spec.
> >>
> >> This patch also adds compile-time checks for variables without precision
> >> information (currently, Mesa only checks that a default precision is set
> >> for floats in fragment shaders).
> >>
> >> As indicated by Ian, the addition of the precision information to
> >> ir_variable has been done using a bitfield and pahole to identify an
> >> available hole so that memory requirements for ir_variable stay the
> >> same.
> >>
> >> v2 (Ian):
> >> - Avoid if-ladders by defining arrays of supported sampler names and
> >> indexing
> >> into them with type->sampler_array + 2 * type->sampler_shadow
> >> - Make the code that selects the precision qualifier to use an utility
> >> function
> >> - Fix a typo
> >>
> >> v3 (Tapani):
> >> - rebased
> >> - squashed in "Precision qualifiers are not allowed on structs"
> >> - fixed select_gles_precision for sampler arrays
> >> - fixed precision_qualifier_allowed for arrays of structs
> >>
> >> v4 (Tapani):
> >> - add atomic_uint handling
> >> - do not allow precision qualifier on images
> >> (issues reported by Marta)
> >>
> >> v5 (Tapani):
> >> - support precision qualifier on image types
> >> ---
> >> src/glsl/ast_to_hir.cpp | 296
> >> ++++++++++++++++++++++++++++++++++++++++----
> >> src/glsl/ir.h | 13 ++
> >> src/glsl/nir/glsl_types.cpp | 4 +
> >> src/glsl/nir/glsl_types.h | 11 ++
> >> 4 files changed, 301 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> >> index b6d662b..1240615 100644
> >> --- a/src/glsl/ast_to_hir.cpp
> >> +++ b/src/glsl/ast_to_hir.cpp
> >> @@ -2189,10 +2189,10 @@ precision_qualifier_allowed(const glsl_type *type)
> >> * From this, we infer that GLSL 1.30 (and later) should allow
> >> precision
> >> * qualifiers on sampler types just like float and integer types.
> >> */
> >> - return type->is_float()
> >> + return (type->is_float()
> >> || type->is_integer()
> >> - || type->is_record()
> >> - || type->contains_opaque();
> >> + || type->contains_opaque())
> >> + && !type->without_array()->is_record();
> >> }
> >>
> >> const glsl_type *
> >> @@ -2210,31 +2210,268 @@ ast_type_specifier::glsl_type(const char **name,
> >> return type;
> >> }
> >>
> >> -const glsl_type *
> >> -ast_fully_specified_type::glsl_type(const char **name,
> >> - struct _mesa_glsl_parse_state *state)
> >> const
> >> +/**
> >> + * From the OpenGL ES 3.0 spec, 4.5.4 Default Precision Qualifiers:
> >> + *
> >> + * "The precision statement
> >> + *
> >> + * precision precision-qualifier type;
> >> + *
> >> + * can be used to establish a default precision qualifier. The type
> >> field can
> >> + * be either int or float or any of the sampler types, (...) If type is
> >> float,
> >> + * the directive applies to non-precision-qualified floating point type
> >> + * (scalar, vector, and matrix) declarations. If type is int, the
> >> directive
> >> + * applies to all non-precision-qualified integer type (scalar, vector,
> >> signed,
> >> + * and unsigned) declarations."
> >> + *
> >> + * We use the symbol table to keep the values of the default precisions
> >> for
> >> + * each 'type' in each scope and we use the 'type' string from the
> >> precision
> >> + * statement as key in the symbol table. When we want to retrieve the
> >> default
> >> + * precision associated with a given glsl_type we need to know the type
> >> string
> >> + * associated with it. This is what this function returns.
> >> + */
> >> +static const char *
> >> +get_type_name_for_precision_qualifier(const glsl_type *type)
> >> {
> >> - const struct glsl_type *type = this->specifier->glsl_type(name, state);
> >> -
> >> - if (type == NULL)
> >> - return NULL;
> >> + switch (type->base_type) {
> >> + case GLSL_TYPE_FLOAT:
> >> + return "float";
> >> + case GLSL_TYPE_UINT:
> >> + case GLSL_TYPE_INT:
> >> + return "int";
> >> + case GLSL_TYPE_ATOMIC_UINT:
> >> + return "atomic_uint";
> >> + case GLSL_TYPE_IMAGE:
> >> + /* fallthrough */
> >
> > I think this is not correct. As far as I understand the spec, we can set
> > a default precision for any of the image types:
> >
> > image2D
> > image3D
> > imageCube
> > image2DArray
> > iimage2D
> > iimage3D
> > iimageCube
> > iimage2DArray
> > uimage2D
> > uimage3D
> > uimageCube
> > uimage2DArray
> >
> > but here you are re-using the precisions from samplers, so if we do
> > this:
> >
> > #precision lowp sampler2D;
> > #precision highp image2D;
> >
> > the latter statement is ignored, and the former affects both samplers
> > and images, which is not what we want.
>
> Note that we don't 'just fallthrough' here but we will return different
> values depending of type->base_type in the switch below. So for
> sampler2D, we will return 'sampler2D' and for image2D 'image2D' because
> of the offset set below:
Oh right, I had missed that. You can add my Ack to your changes.
Iago
> > Iago
> >
> >> + case GLSL_TYPE_SAMPLER: {
> >> + const unsigned type_idx =
> >> + type->sampler_array + 2 * type->sampler_shadow;
> >> + const unsigned offset = type->base_type == GLSL_TYPE_SAMPLER ? 0 :
> >> 4;
> >> + assert(type_idx < 4);
> >> + switch (type->sampler_type) {
> >> + case GLSL_TYPE_FLOAT:
> >> + switch (type->sampler_dimensionality) {
> >> + case GLSL_SAMPLER_DIM_1D: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "sampler1D", "sampler1DArray",
> >> + "sampler1DShadow", "sampler1DArrayShadow"
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_2D: {
> >> + static const char *const names[8] = {
> >> + "sampler2D", "sampler2DArray",
> >> + "sampler2DShadow", "sampler2DArrayShadow",
> >> + "image2D", "image2DArray", NULL, NULL
> >> + };
> >> + return names[offset + type_idx];
>
> In the given example there would be 2 calls to select_gles_precision()
> for both types, here we would return sampler2D for first and image2D for
> second because of the 'offset' used for image.
>
> I wanted to use same switch as same rules apply nicely to sampler and
> image what comes to sampler_dimensionality and sampler_type. Having a
> separate switch for image would be duplication of the switch.
>
> >> + }
> >> + case GLSL_SAMPLER_DIM_3D: {
> >> + static const char *const names[8] = {
> >> + "sampler3D", NULL, NULL, NULL,
> >> + "image3D", NULL, NULL, NULL
> >> + };
> >> + return names[offset + type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_CUBE: {
> >> + static const char *const names[8] = {
> >> + "samplerCube", "samplerCubeArray",
> >> + "samplerCubeShadow", "samplerCubeArrayShadow",
> >> + "imageCube", NULL, NULL, NULL
> >> + };
> >> + return names[offset + type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_MS: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "sampler2DMS", "sampler2DMSArray", NULL, NULL
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_RECT: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "samplerRect", NULL, "samplerRectShadow", NULL
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_BUF: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "samplerBuffer", NULL, NULL, NULL
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_EXTERNAL: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "samplerExternalOES", NULL, NULL, NULL
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + default:
> >> + unreachable("Unsupported sampler/image dimensionality");
> >> + } /* sampler/image float dimensionality */
> >> + break;
> >> + case GLSL_TYPE_INT:
> >> + switch (type->sampler_dimensionality) {
> >> + case GLSL_SAMPLER_DIM_1D: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "isampler1D", "isampler1DArray", NULL, NULL
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_2D: {
> >> + static const char *const names[8] = {
> >> + "isampler2D", "isampler2DArray", NULL, NULL,
> >> + "iimage2D", "iimage2DArray", NULL, NULL
> >> + };
> >> + return names[offset + type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_3D: {
> >> + static const char *const names[8] = {
> >> + "isampler3D", NULL, NULL, NULL,
> >> + "iimage3D", NULL, NULL, NULL
> >> + };
> >> + return names[offset + type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_CUBE: {
> >> + static const char *const names[8] = {
> >> + "isamplerCube", "isamplerCubeArray", NULL, NULL,
> >> + "iimageCube", NULL, NULL, NULL
> >> + };
> >> + return names[offset + type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_MS: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "isampler2DMS", "isampler2DMSArray", NULL, NULL
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_RECT: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "isamplerRect", NULL, "isamplerRectShadow", NULL
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_BUF: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "isamplerBuffer", NULL, NULL, NULL
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + default:
> >> + unreachable("Unsupported isampler/iimage dimensionality");
> >> + } /* sampler/image int dimensionality */
> >> + break;
> >> + case GLSL_TYPE_UINT:
> >> + switch (type->sampler_dimensionality) {
> >> + case GLSL_SAMPLER_DIM_1D: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "usampler1D", "usampler1DArray", NULL, NULL
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_2D: {
> >> + static const char *const names[8] = {
> >> + "usampler2D", "usampler2DArray", NULL, NULL,
> >> + "uimage2D", "uimage2DArray", NULL, NULL,
> >> + };
> >> + return names[offset + type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_3D: {
> >> + static const char *const names[8] = {
> >> + "usampler3D", NULL, NULL, NULL,
> >> + "uimage3D", NULL, NULL, NULL,
> >> + };
> >> + return names[offset + type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_CUBE: {
> >> + static const char *const names[8] = {
> >> + "usamplerCube", "usamplerCubeArray", NULL, NULL,
> >> + "uimageCube", NULL, NULL, NULL
> >> + };
> >> + return names[offset + type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_MS: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "usampler2DMS", "usampler2DMSArray", NULL, NULL
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_RECT: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "usamplerRect", NULL, "usamplerRectShadow", NULL
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + case GLSL_SAMPLER_DIM_BUF: {
> >> + assert(type->base_type == GLSL_TYPE_SAMPLER);
> >> + static const char *const names[4] = {
> >> + "usamplerBuffer", NULL, NULL, NULL
> >> + };
> >> + return names[type_idx];
> >> + }
> >> + default:
> >> + unreachable("Unsupported usampler/uimage dimensionality");
> >> + } /* sampler/image uint dimensionality */
> >> + break;
> >> + default:
> >> + unreachable("Unsupported sampler/image type");
> >> + } /* sampler/image type */
> >> + break;
> >> + } /* GLSL_TYPE_SAMPLER/GLSL_TYPE_IMAGE */
> >> + break;
> >> + default:
> >> + unreachable("Unsupported type");
> >> + } /* base type */
> >> +}
> >>
> >> - /* The fragment language does not define a default precision value
> >> - * for float types, so check that one is defined if the type
> >> declaration
> >> - * isn't providing one explictly.
> >> +static unsigned
> >> +select_gles_precision(unsigned qual_precision,
> >> + const glsl_type *type,
> >> + struct _mesa_glsl_parse_state *state, YYLTYPE *loc)
> >> +{
> >> + /* Precision qualifiers do not have any meaning in Desktop GLSL.
> >> + * In GLES we take the precision from the type qualifier if present,
> >> + * otherwise, if the type of the variable allows precision qualifiers
> >> at
> >> + * all, we look for the default precision qualifier for that type in
> >> the
> >> + * current scope.
> >> */
> >> - if (type->base_type == GLSL_TYPE_FLOAT
> >> - && state->es_shader
> >> - && state->stage == MESA_SHADER_FRAGMENT
> >> - && this->qualifier.precision == ast_precision_none
> >> - && state->symbols->get_default_precision_qualifier("float") ==
> >> ast_precision_none) {
> >> - YYLTYPE loc = this->get_location();
> >> - _mesa_glsl_error(&loc, state,
> >> - "no precision specified this scope for type `%s'",
> >> - type->name);
> >> + assert(state->es_shader);
> >> +
> >> + unsigned precision = GLSL_PRECISION_NONE;
> >> + if (qual_precision) {
> >> + precision = qual_precision;
> >> + } else if (precision_qualifier_allowed(type)) {
> >> + const char *type_name =
> >> + get_type_name_for_precision_qualifier(type->without_array());
> >> + assert(type_name != NULL);
> >> +
> >> + precision =
> >> + state->symbols->get_default_precision_qualifier(type_name);
> >> + if (precision == ast_precision_none) {
> >> + _mesa_glsl_error(loc, state,
> >> + "No precision specified in this scope for type
> >> `%s'",
> >> + type->name);
> >> + }
> >> }
> >> + return precision;
> >> +}
> >>
> >> - return type;
> >> +const glsl_type *
> >> +ast_fully_specified_type::glsl_type(const char **name,
> >> + struct _mesa_glsl_parse_state *state)
> >> const
> >> +{
> >> + return this->specifier->glsl_type(name, state);
> >> }
> >>
> >> /**
> >> @@ -2772,6 +3009,12 @@ 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) {
> >> + var->data.precision =
> >> + select_gles_precision(qual->precision, var->type, state, loc);
> >> + }
> >> +
> >> if (state->stage == MESA_SHADER_GEOMETRY &&
> >> qual->flags.q.out && qual->flags.q.stream) {
> >> var->data.stream = qual->stream;
> >> @@ -6635,6 +6878,13 @@ ast_interface_block::hir(exec_list *instructions,
> >> if (var_mode == ir_var_shader_in || var_mode == ir_var_uniform)
> >> var->data.read_only = true;
> >>
> >> + /* Precision qualifiers do not have any meaning in Desktop GLSL
> >> */
> >> + if (state->es_shader) {
> >> + var->data.precision =
> >> + select_gles_precision(fields[i].precision, fields[i].type,
> >> + state, &loc);
> >> + }
> >> +
> >> 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/ir.h b/src/glsl/ir.h
> >> index 9c9f22d..b90451c 100644
> >> --- a/src/glsl/ir.h
> >> +++ b/src/glsl/ir.h
> >> @@ -770,6 +770,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/nir/glsl_types.cpp b/src/glsl/nir/glsl_types.cpp
> >> index 1c66dce..975b815 100644
> >> --- a/src/glsl/nir/glsl_types.cpp
> >> +++ b/src/glsl/nir/glsl_types.cpp
> >> @@ -162,6 +162,7 @@ glsl_type::glsl_type(const glsl_struct_field *fields,
> >> unsigned num_fields,
> >> this->fields.structure[i].sample = fields[i].sample;
> >> this->fields.structure[i].matrix_layout = fields[i].matrix_layout;
> >> this->fields.structure[i].patch = fields[i].patch;
> >> + this->fields.structure[i].precision = fields[i].precision;
> >> }
> >>
> >> mtx_unlock(&glsl_type::mutex);
> >> @@ -779,6 +780,9 @@ glsl_type::record_compare(const glsl_type *b) const
> >> if (this->fields.structure[i].image_restrict
> >> != b->fields.structure[i].image_restrict)
> >> return false;
> >> + if (this->fields.structure[i].precision
> >> + != b->fields.structure[i].precision)
> >> + return false;
> >> }
> >>
> >> return true;
> >> diff --git a/src/glsl/nir/glsl_types.h b/src/glsl/nir/glsl_types.h
> >> index 52ca826..54e5c47 100644
> >> --- a/src/glsl/nir/glsl_types.h
> >> +++ b/src/glsl/nir/glsl_types.h
> >> @@ -102,6 +102,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"
> >> @@ -834,6 +841,10 @@ struct glsl_struct_field {
> >> */
> >> int stream;
> >>
> >> + /**
> >> + * Precision qualifier
> >> + */
> >> + unsigned precision;
> >>
> >> /**
> >> * Image qualifiers, applicable to buffer variables defined in shader
> >
> >
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
