Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
On Mon, Nov 16, 2015 at 11:44 AM, Ilia Mirkin wrote: > On Mon, Nov 16, 2015 at 11:42 AM, Samuel Iglesias Gonsálvez > wrote: >> >> >> On 16/11/15 17:34, Ilia Mirkin wrote: >>> On Mon, Nov 16, 2015 at 11:29 AM, Samuel Iglesias Gonsálvez >>> wrote: On 16/11/15 13:07, Tapani Pälli wrote: > > On 11/16/2015 01:35 PM, Tapani Pälli wrote: >> >> >> On 11/16/2015 01:29 PM, Samuel Iglesias Gonsálvez wrote: >>> Hello Ilia, Tapani: >>> >>> I have reproduced the issue with a piglit test but not with the trace >>> uploaded in the bug report :-( >>> >>> The piglit test was: bin/arb_shader_storage_buffer_object-maxblocks >>> >>> I have upload a branch with some fixes at Igalia's mesa repo: >>> >>> Git repo: https://github.com/Igalia/mesa.git >>> Branch: wip/siglesias/precision-fixes >>> >>> But as this error might come from other initializations that I might >>> overlook: >>> * Ilia: Could you test if this issue is still happening to you? As I >>> cannot reproduce it locally, I might be forgetting something. >>> * Tapani: Could you do a quick run on CTS to check I have not broken >>> anything? >> >> Sure thing, I'll run testing. FWIW one of the patches was identical to >> my fix sent for fixing tessellation shader problems: >> >> http://lists.freedesktop.org/archives/mesa-dev/2015-November/100396.html > > No CTS regressions with these patches, I've gone through these and > changes look good to me! > > OK, once Ilia replies that the issue is fixed with those patches, I will send them for review to the mailing list :-) >>> >>> I won't have time to look until tonight. However the repro steps were >>> pretty simple... download the trace and run through valgrind. Probably >>> tons of other ways to trigger it too, of course... I'd esp look for >>> piglits that have uniform structs. >>> >> >> The problem is that I could not reproduce it with the trace. That's why >> I am asking. >> >> I reproduce it with a piglit tests, but maybe precision is uninitialized >> in other cases. Tomorrow I will do some more testing, just in case. > > Well, irrespective of other cases, if the things you're fixing are > real fixes, no need to wait on me. I'll be sure to complain again if I > still see problems. FWIW I did see them with nouveau, not i965. I > suspect llvmpipe would take the same paths. With all the latest patches that have landed, valgrind is happy again. [At least in regards to this... it's got complaints about other things, but those aren't your fault.] Thanks for fixing! -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
On 16/11/15 17:34, Ilia Mirkin wrote: > On Mon, Nov 16, 2015 at 11:29 AM, Samuel Iglesias Gonsálvez > wrote: >> >> >> On 16/11/15 13:07, Tapani Pälli wrote: >>> >>> On 11/16/2015 01:35 PM, Tapani Pälli wrote: On 11/16/2015 01:29 PM, Samuel Iglesias Gonsálvez wrote: > Hello Ilia, Tapani: > > I have reproduced the issue with a piglit test but not with the trace > uploaded in the bug report :-( > > The piglit test was: bin/arb_shader_storage_buffer_object-maxblocks > > I have upload a branch with some fixes at Igalia's mesa repo: > > Git repo: https://github.com/Igalia/mesa.git > Branch: wip/siglesias/precision-fixes > > But as this error might come from other initializations that I might > overlook: > * Ilia: Could you test if this issue is still happening to you? As I > cannot reproduce it locally, I might be forgetting something. > * Tapani: Could you do a quick run on CTS to check I have not broken > anything? Sure thing, I'll run testing. FWIW one of the patches was identical to my fix sent for fixing tessellation shader problems: http://lists.freedesktop.org/archives/mesa-dev/2015-November/100396.html >>> >>> No CTS regressions with these patches, I've gone through these and >>> changes look good to me! >>> >>> >> >> OK, once Ilia replies that the issue is fixed with those patches, I will >> send them for review to the mailing list :-) > > I won't have time to look until tonight. However the repro steps were > pretty simple... download the trace and run through valgrind. Probably > tons of other ways to trigger it too, of course... I'd esp look for > piglits that have uniform structs. > The problem is that I could not reproduce it with the trace. That's why I am asking. I reproduce it with a piglit tests, but maybe precision is uninitialized in other cases. Tomorrow I will do some more testing, just in case. Thanks, Sam ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
On Mon, Nov 16, 2015 at 11:42 AM, Samuel Iglesias Gonsálvez wrote: > > > On 16/11/15 17:34, Ilia Mirkin wrote: >> On Mon, Nov 16, 2015 at 11:29 AM, Samuel Iglesias Gonsálvez >> wrote: >>> >>> >>> On 16/11/15 13:07, Tapani Pälli wrote: On 11/16/2015 01:35 PM, Tapani Pälli wrote: > > > On 11/16/2015 01:29 PM, Samuel Iglesias Gonsálvez wrote: >> Hello Ilia, Tapani: >> >> I have reproduced the issue with a piglit test but not with the trace >> uploaded in the bug report :-( >> >> The piglit test was: bin/arb_shader_storage_buffer_object-maxblocks >> >> I have upload a branch with some fixes at Igalia's mesa repo: >> >> Git repo: https://github.com/Igalia/mesa.git >> Branch: wip/siglesias/precision-fixes >> >> But as this error might come from other initializations that I might >> overlook: >> * Ilia: Could you test if this issue is still happening to you? As I >> cannot reproduce it locally, I might be forgetting something. >> * Tapani: Could you do a quick run on CTS to check I have not broken >> anything? > > Sure thing, I'll run testing. FWIW one of the patches was identical to > my fix sent for fixing tessellation shader problems: > > http://lists.freedesktop.org/archives/mesa-dev/2015-November/100396.html No CTS regressions with these patches, I've gone through these and changes look good to me! >>> >>> OK, once Ilia replies that the issue is fixed with those patches, I will >>> send them for review to the mailing list :-) >> >> I won't have time to look until tonight. However the repro steps were >> pretty simple... download the trace and run through valgrind. Probably >> tons of other ways to trigger it too, of course... I'd esp look for >> piglits that have uniform structs. >> > > The problem is that I could not reproduce it with the trace. That's why > I am asking. > > I reproduce it with a piglit tests, but maybe precision is uninitialized > in other cases. Tomorrow I will do some more testing, just in case. Well, irrespective of other cases, if the things you're fixing are real fixes, no need to wait on me. I'll be sure to complain again if I still see problems. FWIW I did see them with nouveau, not i965. I suspect llvmpipe would take the same paths. -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
On Mon, Nov 16, 2015 at 11:29 AM, Samuel Iglesias Gonsálvez wrote: > > > On 16/11/15 13:07, Tapani Pälli wrote: >> >> On 11/16/2015 01:35 PM, Tapani Pälli wrote: >>> >>> >>> On 11/16/2015 01:29 PM, Samuel Iglesias Gonsálvez wrote: Hello Ilia, Tapani: I have reproduced the issue with a piglit test but not with the trace uploaded in the bug report :-( The piglit test was: bin/arb_shader_storage_buffer_object-maxblocks I have upload a branch with some fixes at Igalia's mesa repo: Git repo: https://github.com/Igalia/mesa.git Branch: wip/siglesias/precision-fixes But as this error might come from other initializations that I might overlook: * Ilia: Could you test if this issue is still happening to you? As I cannot reproduce it locally, I might be forgetting something. * Tapani: Could you do a quick run on CTS to check I have not broken anything? >>> >>> Sure thing, I'll run testing. FWIW one of the patches was identical to >>> my fix sent for fixing tessellation shader problems: >>> >>> http://lists.freedesktop.org/archives/mesa-dev/2015-November/100396.html >> >> No CTS regressions with these patches, I've gone through these and >> changes look good to me! >> >> > > OK, once Ilia replies that the issue is fixed with those patches, I will > send them for review to the mailing list :-) I won't have time to look until tonight. However the repro steps were pretty simple... download the trace and run through valgrind. Probably tons of other ways to trigger it too, of course... I'd esp look for piglits that have uniform structs. -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
On 16/11/15 13:07, Tapani Pälli wrote: > > On 11/16/2015 01:35 PM, Tapani Pälli wrote: >> >> >> On 11/16/2015 01:29 PM, Samuel Iglesias Gonsálvez wrote: >>> Hello Ilia, Tapani: >>> >>> I have reproduced the issue with a piglit test but not with the trace >>> uploaded in the bug report :-( >>> >>> The piglit test was: bin/arb_shader_storage_buffer_object-maxblocks >>> >>> I have upload a branch with some fixes at Igalia's mesa repo: >>> >>> Git repo: https://github.com/Igalia/mesa.git >>> Branch: wip/siglesias/precision-fixes >>> >>> But as this error might come from other initializations that I might >>> overlook: >>> * Ilia: Could you test if this issue is still happening to you? As I >>> cannot reproduce it locally, I might be forgetting something. >>> * Tapani: Could you do a quick run on CTS to check I have not broken >>> anything? >> >> Sure thing, I'll run testing. FWIW one of the patches was identical to >> my fix sent for fixing tessellation shader problems: >> >> http://lists.freedesktop.org/archives/mesa-dev/2015-November/100396.html > > No CTS regressions with these patches, I've gone through these and > changes look good to me! > > OK, once Ilia replies that the issue is fixed with those patches, I will send them for review to the mailing list :-) Thanks! Sam >>> Thanks! >>> >>> Sam >>> >> ___ >> 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
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
On 11/16/2015 01:35 PM, Tapani Pälli wrote: On 11/16/2015 01:29 PM, Samuel Iglesias Gonsálvez wrote: Hello Ilia, Tapani: I have reproduced the issue with a piglit test but not with the trace uploaded in the bug report :-( The piglit test was: bin/arb_shader_storage_buffer_object-maxblocks I have upload a branch with some fixes at Igalia's mesa repo: Git repo: https://github.com/Igalia/mesa.git Branch: wip/siglesias/precision-fixes But as this error might come from other initializations that I might overlook: * Ilia: Could you test if this issue is still happening to you? As I cannot reproduce it locally, I might be forgetting something. * Tapani: Could you do a quick run on CTS to check I have not broken anything? Sure thing, I'll run testing. FWIW one of the patches was identical to my fix sent for fixing tessellation shader problems: http://lists.freedesktop.org/archives/mesa-dev/2015-November/100396.html No CTS regressions with these patches, I've gone through these and changes look good to me! Thanks! Sam ___ 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
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
On 16/11/15 12:35, Tapani Pälli wrote: > > > On 11/16/2015 01:29 PM, Samuel Iglesias Gonsálvez wrote: >> Hello Ilia, Tapani: >> >> I have reproduced the issue with a piglit test but not with the trace >> uploaded in the bug report :-( >> >> The piglit test was: bin/arb_shader_storage_buffer_object-maxblocks >> >> I have upload a branch with some fixes at Igalia's mesa repo: >> >> Git repo: https://github.com/Igalia/mesa.git >> Branch: wip/siglesias/precision-fixes >> >> But as this error might come from other initializations that I might >> overlook: >> * Ilia: Could you test if this issue is still happening to you? As I >> cannot reproduce it locally, I might be forgetting something. >> * Tapani: Could you do a quick run on CTS to check I have not broken >> anything? > > Sure thing, I'll run testing. FWIW one of the patches was identical to > my fix sent for fixing tessellation shader problems: > > http://lists.freedesktop.org/archives/mesa-dev/2015-November/100396.html > OK, thanks. I have reviewed your patch. Sam ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
On 11/16/2015 01:29 PM, Samuel Iglesias Gonsálvez wrote: Hello Ilia, Tapani: I have reproduced the issue with a piglit test but not with the trace uploaded in the bug report :-( The piglit test was: bin/arb_shader_storage_buffer_object-maxblocks I have upload a branch with some fixes at Igalia's mesa repo: Git repo: https://github.com/Igalia/mesa.git Branch: wip/siglesias/precision-fixes But as this error might come from other initializations that I might overlook: * Ilia: Could you test if this issue is still happening to you? As I cannot reproduce it locally, I might be forgetting something. * Tapani: Could you do a quick run on CTS to check I have not broken anything? Sure thing, I'll run testing. FWIW one of the patches was identical to my fix sent for fixing tessellation shader problems: http://lists.freedesktop.org/archives/mesa-dev/2015-November/100396.html Thanks! Sam ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
Hello Ilia, Tapani: I have reproduced the issue with a piglit test but not with the trace uploaded in the bug report :-( The piglit test was: bin/arb_shader_storage_buffer_object-maxblocks I have upload a branch with some fixes at Igalia's mesa repo: Git repo: https://github.com/Igalia/mesa.git Branch: wip/siglesias/precision-fixes But as this error might come from other initializations that I might overlook: * Ilia: Could you test if this issue is still happening to you? As I cannot reproduce it locally, I might be forgetting something. * Tapani: Could you do a quick run on CTS to check I have not broken anything? Thanks! Sam ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
On 13/11/15 21:38, Ilia Mirkin wrote: > On Fri, Nov 13, 2015 at 2:37 PM, Ilia Mirkin wrote: >> Looks like valgrind hates this for some reason. I'm seeing lots of >> >> ==16821== Conditional jump or move depends on uninitialised value(s) >> ==16821==at 0xA074D09: glsl_type::record_compare(glsl_type const*) >> const (glsl_types.cpp:783) >> >> Where line 783 is: >> >> if (this->fields.structure[i].precision >> != b->fields.structure[i].precision) >> >> This happens with the trace from >> https://bugs.freedesktop.org/show_bug.cgi?id=92229 but I suspect it >> happens with just about anything with structs. > > I tried the following but no go. I'm giving up for now. > OK, I can reproduce this valgrind error. I am going to debug it. Thanks! Sam > -ilia > > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp > index 51ea183..92f8b37 100644 > --- a/src/glsl/ast_to_hir.cpp > +++ b/src/glsl/ast_to_hir.cpp > @@ -6584,6 +6584,8 @@ ast_interface_block::hir(exec_list *instructions, > earlier_per_vertex->fields.structure[j].sample; > fields[i].patch = > earlier_per_vertex->fields.structure[j].patch; > +fields[i].precision = > + earlier_per_vertex->fields.structure[j].precision; > } >} > > diff --git a/src/glsl/nir/glsl_types.cpp b/src/glsl/nir/glsl_types.cpp > index 975b815..7345765 100644 > --- a/src/glsl/nir/glsl_types.cpp > +++ b/src/glsl/nir/glsl_types.cpp > @@ -124,6 +124,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; >this->fields.structure[i].image_read_only = fields[i].image_read_only; >this->fields.structure[i].image_write_only = > fields[i].image_write_only; >this->fields.structure[i].image_coherent = fields[i].image_coherent; > diff --git a/src/glsl/nir/glsl_types.h b/src/glsl/nir/glsl_types.h > index d841a32..f3a0cf8 100644 > --- a/src/glsl/nir/glsl_types.h > +++ b/src/glsl/nir/glsl_types.h > @@ -851,7 +851,7 @@ struct glsl_struct_field { > > glsl_struct_field(const struct glsl_type *_type, const char *_name) >: type(_type), name(_name), location(-1), interpolation(0), > centroid(0), > -sample(0), matrix_layout(GLSL_MATRIX_LAYOUT_INHERITED), patch(0) > +sample(0), matrix_layout(GLSL_MATRIX_LAYOUT_INHERITED), > patch(0), precision(0) > { >/* empty */ > } > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
On Fri, Nov 13, 2015 at 2:37 PM, Ilia Mirkin wrote: > Looks like valgrind hates this for some reason. I'm seeing lots of > > ==16821== Conditional jump or move depends on uninitialised value(s) > ==16821==at 0xA074D09: glsl_type::record_compare(glsl_type const*) > const (glsl_types.cpp:783) > > Where line 783 is: > > if (this->fields.structure[i].precision > != b->fields.structure[i].precision) > > This happens with the trace from > https://bugs.freedesktop.org/show_bug.cgi?id=92229 but I suspect it > happens with just about anything with structs. I tried the following but no go. I'm giving up for now. -ilia diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp index 51ea183..92f8b37 100644 --- a/src/glsl/ast_to_hir.cpp +++ b/src/glsl/ast_to_hir.cpp @@ -6584,6 +6584,8 @@ ast_interface_block::hir(exec_list *instructions, earlier_per_vertex->fields.structure[j].sample; fields[i].patch = earlier_per_vertex->fields.structure[j].patch; +fields[i].precision = + earlier_per_vertex->fields.structure[j].precision; } } diff --git a/src/glsl/nir/glsl_types.cpp b/src/glsl/nir/glsl_types.cpp index 975b815..7345765 100644 --- a/src/glsl/nir/glsl_types.cpp +++ b/src/glsl/nir/glsl_types.cpp @@ -124,6 +124,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; this->fields.structure[i].image_read_only = fields[i].image_read_only; this->fields.structure[i].image_write_only = fields[i].image_write_only; this->fields.structure[i].image_coherent = fields[i].image_coherent; diff --git a/src/glsl/nir/glsl_types.h b/src/glsl/nir/glsl_types.h index d841a32..f3a0cf8 100644 --- a/src/glsl/nir/glsl_types.h +++ b/src/glsl/nir/glsl_types.h @@ -851,7 +851,7 @@ struct glsl_struct_field { glsl_struct_field(const struct glsl_type *_type, const char *_name) : type(_type), name(_name), location(-1), interpolation(0), centroid(0), -sample(0), matrix_layout(GLSL_MATRIX_LAYOUT_INHERITED), patch(0) +sample(0), matrix_layout(GLSL_MATRIX_LAYOUT_INHERITED), patch(0), precision(0) { /* empty */ } ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
Looks like valgrind hates this for some reason. I'm seeing lots of ==16821== Conditional jump or move depends on uninitialised value(s) ==16821==at 0xA074D09: glsl_type::record_compare(glsl_type const*) const (glsl_types.cpp:783) Where line 783 is: if (this->fields.structure[i].precision != b->fields.structure[i].precision) This happens with the trace from https://bugs.freedesktop.org/show_bug.cgi?id=92229 but I suspect it happens with just about anything with structs. -ilia On Wed, Nov 11, 2015 at 7:45 AM, Samuel Iglesias Gonsálvez wrote: > Reviewed-by: Samuel Iglesias Gonsálvez > > On 06/11/15 13:03, Tapani Pälli wrote: >> From: Iago Toral Quiroga >> >> 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 */ >> + case GLSL_TYPE_SAMPLER: { >> + const unsigned type_idx = >> + type->sampler_array + 2 * type->sampler_shadow; >> + const unsigned offset = type->base_type == GLSL_TYPE_
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
Reviewed-by: Samuel Iglesias Gonsálvez On 06/11/15 13:03, Tapani Pälli wrote: > From: Iago Toral Quiroga > > 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 */ > + 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", "sampler2
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
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 > >> > >> 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 lo
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
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 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: 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->sam
Re: [Mesa-dev] [PATCH v5 5/7] glsl: Add precision information to ir_variable
On Fri, 2015-11-06 at 14:03 +0200, Tapani Pälli wrote: > From: Iago Toral Quiroga > > 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. 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: { > +