[Mesa-dev] [PATCH 19/19] glsl: Allow arrays of arrays in GLSL ES 3.10 and GLSL 4.30

[Timothy Arceri]

On Sat, 2015-06-20 at 23:57 -0400, Ilia Mirkin wrote:
> On Sat, Jun 20, 2015 at 11:38 PM, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> > On Sat, 2015-06-20 at 19:35 -0400, Ilia Mirkin wrote:
> >> On Sat, Jun 20, 2015 at 8:33 AM, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> >> > ---
> >> >  src/glsl/ast_to_hir.cpp       | 13 ++++++++-----
> >> >  src/glsl/glsl_parser.yy       | 22 ++++++++++++++--------
> >> >  src/glsl/glsl_parser_extras.h |  5 +++++
> >> >  3 files changed, 27 insertions(+), 13 deletions(-)
> >> >
> >> > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> >> > index 6d2dc2e..81b2765 100644
> >> > --- a/src/glsl/ast_to_hir.cpp
> >> > +++ b/src/glsl/ast_to_hir.cpp
> >> > @@ -1939,12 +1939,15 @@ process_array_type(YYLTYPE *loc, const glsl_type *base,
> >> >            *
> >> >            * "Only one-dimensional arrays may be declared."
> >> >            */
> >> > -         if (!state->ARB_arrays_of_arrays_enable) {
> >> > +         if (!state->has_arrays_of_arrays()) {
> >> > +            const char *const requirement = state->es_shader
> >> > +               ? "GLSL ES 310"
> >> > +               : "GL_ARB_arrays_of_array or GLSL 430";
> >>
> >> I think everywhere I've seen it's GLSL ES 3.10 and GLSL 4.30. Also, it
> >> should be arrays_of_array*s*
> >
> > Thanks for pointing out the missing 's' I'll fix that, however it seems
> > more common to use GLSL ES 310 and GLSL 430 see glsl_parser_extras.h.
> >
> > I could only find one instance of "GLSL 4.30" in glsl_parser.yy I
> > searched from 3.30-4.50
> 
> $ git grep -P 'GLSL (ES )?\d{3}'
> 
> reveals just a handful of instances:
> 
> src/gallium/drivers/freedreno/freedreno_screen.c:
> {"glsl120",   FD_DBG_GLSL120,"Temporary flag to force GLSL 120 (rather
> than 130) on a3xx+"},
> src/glsl/glsl_parser_extras.h:         const char *const requirement =
> "GL_ARB_gpu_shader5 extension or GLSL 400";
> src/glsl/glsl_parser_extras.h:            ? "GLSL ES 300"
> src/glsl/glsl_parser_extras.h:            :
> "GL_ARB_explicit_attrib_location extension or GLSL 330";
> src/glsl/glsl_parser_extras.h:            ?
> "GL_EXT_separate_shader_objects extension or GLSL ES 310"
> src/glsl/glsl_parser_extras.h:            :
> "GL_ARB_separate_shader_objects extension or GLSL 420";
> src/glsl/glsl_parser_extras.h:            ? "GLSL ES 310"
> src/glsl/glsl_parser_extras.h:
> "GL_ARB_explicit_attrib_location or GLSL 330.";
> 
> Which IMHO should be fixed. Compare that to
> 
> $ git grep -P 'GLSL (ES )?\d{1}\.\d{2}' src | wc -l
> 286
> 
> Most of which come from comments... I guess there are just a few that
> end up in user strings:
> 
> src/glsl/ast_array_index.cpp:                             "expressions
> will be forbidden in GLSL 1.30 "
> src/glsl/ast_array_index.cpp:                        "expressions is
> forbidden in GLSL 1.30 and "
> src/glsl/ast_to_hir.cpp:                          "function `%s' in
> GLSL ES 3.00", name);
> src/glsl/ast_to_hir.cpp:                          "not allowed in GLSL
> ES 1.00");
> src/glsl/glsl_parser.yy:                            "(GLSL ES 1.00 or
> GLSL 1.20 required)",
> src/glsl/glsl_parser.yy:                                "%s qualifier
> requires GLSL 4.30 or "
> src/glsl/glsl_parser_extras.cpp:                          "GLSL 1.00
> ES should be selected using "
> 
> The version strings reported also have the . in them:
> 
> const char *
> glsl_compute_version_string(void *mem_ctx, bool is_es, unsigned version)
> {
>    return ralloc_asprintf(mem_ctx, "GLSL%s %d.%02d", is_es ? " ES" : "",
>                           version / 100, version % 100);
> }
> 
> IMHO it'd be better to standardize on one thing.

Yes I agree they should be all the same. I'll make a patch to do that
and see what others have to say about it.

> 
>   -ilia