[Mesa-dev] [PATCH v3 12/14] glsl: add support for complie-time constant expressions

[Timothy Arceri]

On Tue, 2015-11-17 at 19:02 +0000, Emil Velikov wrote:
> On 14 November 2015 at 13:42, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> > From: Timothy Arceri <timothy.arceri at collabora.com>
> > 
> > This patch replaces the old interger constant qualifiers with either
> > the new ast_layout_expression type if the qualifier requires merging
> > or ast_expression if the qualifier can't have mulitple declarations
> > or if all but the newest qualifier is simply ignored.
> > 
> > We also update the process_qualifier_constant() helper to be
> > similar to the one in the ast_layout_expression class, but in
> > this case it will be used to process the ast_expression qualifiers.
> > 
> > Global shader layout qualifier validation is moved out of the parser
> > in this change as we now need to evaluate any constant expression
> > before doing the validation.
> > ---
> >  src/glsl/ast.h                  |  33 +++++------
> >  src/glsl/ast_to_hir.cpp         | 126 ++++++++++++++++++++++++++++-------
> > -----
> >  src/glsl/ast_type.cpp           |  69 ++++++++--------------
> >  src/glsl/glsl_parser.yy         |  87 +++++++++------------------
> >  src/glsl/glsl_parser_extras.cpp |  44 ++++++++++++--
> >  5 files changed, 195 insertions(+), 164 deletions(-)
> > 
> 
> > --- a/src/glsl/ast_to_hir.cpp
> > +++ b/src/glsl/ast_to_hir.cpp
> 
> > @@ -4079,9 +4113,18 @@ ast_declarator_list::hir(exec_list *instructions,
> >      */
> >     if (decl_type && decl_type->contains_atomic()) {
> >        if (type->qualifier.flags.q.explicit_binding &&
> > -          type->qualifier.flags.q.explicit_offset)
> > -         state->atomic_counter_offsets[type->qualifier.binding] =
> > -            type->qualifier.offset;
> > +          type->qualifier.flags.q.explicit_offset) {
> > +         unsigned qual_binding;
> > +         unsigned qual_offset;
> > +         if (process_qualifier_constant(state, &loc, "binding",
> > +                                        type->qualifier.binding,
> > +                                        &qual_binding)
> > +             && process_qualifier_constant(state, &loc, "offset",
> Nitpick: Please leave the && trailing on the previous line.
> 
> > --- a/src/glsl/glsl_parser.yy
> > +++ b/src/glsl/glsl_parser.yy
> 
> >        /* Layout qualifiers for tessellation control shaders. */
> >        if (match_layout_qualifier("vertices", $1, state) == 0) {
> >           $$.flags.q.vertices = 1;
> > -
> > -         if ($3 <= 0) {
> > -            _mesa_glsl_error(& @3, state,
> > -                             "invalid vertices (%d) specified", $3);
> > -            YYERROR;
> > -         } else if ($3 > (int)state->Const.MaxPatchVertices) {
> > -            _mesa_glsl_error(& @3, state,
> > -                             "vertices (%d) exceeds "
> > -                             "GL_MAX_PATCH_VERTICES", $3);
> > -            YYERROR;
> > -         } else {
> > -            $$.vertices = $3;
> > -            if (!state->ARB_tessellation_shader_enable &&
> > -                !state->is_version(400, 0)) {
> > -               _mesa_glsl_error(& @1, state,
> > -                                "vertices qualifier requires GLSL 4.00 or
> > "
> > -                                "ARB_tessellation_shader");
> > -            }
> > +         $$.vertices = new(ctx) ast_layout_expression(@1, $3);
> > +         if (!state->ARB_tessellation_shader_enable &&
> > +             !state->is_version(400, 0)) {
> > +            _mesa_glsl_error(& @1, state,
> > +                             "vertices qualifier requires GLSL 4.00 or "
> > +                             "ARB_tessellation_shader");
> >           }
> >        }
> > 
> > diff --git a/src/glsl/glsl_parser_extras.cpp
> > b/src/glsl/glsl_parser_extras.cpp
> > index 1678d89..2f870fc 100644
> > --- a/src/glsl/glsl_parser_extras.cpp
> > +++ b/src/glsl/glsl_parser_extras.cpp
> > @@ -1644,8 +1644,20 @@ set_shader_inout_layout(struct gl_shader *shader,
> >     switch (shader->Stage) {
> >     case MESA_SHADER_TESS_CTRL:
> >        shader->TessCtrl.VerticesOut = 0;
> > -      if (state->tcs_output_vertices_specified)
> > -         shader->TessCtrl.VerticesOut = state->out_qualifier->vertices;
> > +      if (state->tcs_output_vertices_specified) {
> > +         unsigned vertices;
> > +         if (state->out_qualifier->vertices->
> > +               process_qualifier_constant(state, "vertices", &vertices,
> > +                                          true)) {
> The existing code considering 0 an invalid amount, which afaict that
> is incorrect. Splitting that into a separate patch is an overkill,
> although mentioning it in the commit message is a (almost) must have
> imho.

Nope this is my bug thanks for spotting :) I'll fix this locally. I'm sure I
double checked all these before sending out. In this case I'm sure I changed
this value, maybe I changed it to the wrong value.

> 
> -Emil