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

[Emil Velikov]

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.

-Emil