[Mesa-dev] [PATCH 4/6] glsl: add support for complie-time constant expressions

Emil Velikov emil.l.velikov at gmail.com
Fri Nov 6 06:17:51 PST 2015


Hi Tim,

In my (limited) experience going through the glsl code, I've noticed
that in a few cases we tend to do the same "flags.q.foo &&
validate_foo()" in a number of occasions. In some cases we silently
ignore errors, while on others we omit checking the return value of
validate_foo() all together.

I'm leaning that at least some of these look fishy, although I would
appreciate any feedback (from everyone for that matter).
Should we try to consolidate all the above pattern(s), where is the
clear cut when we can and cannot continue if the validation fails ?

A couple of related (and other) comments inline.

On 5 November 2015 at 11:17, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> From: Timothy Arceri <timothy.arceri at collabora.com>
>
> In this patch we introduce a new ast type for holding the new
> compile-time constant expressions. The main reason for this is that
> we can no longer do merging of layout qualifiers before they have been
> converted into GLSL IR.
>
> The remainder of the patch replaces all 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 declorations or if all but he newest qualifier
> is simply ignored.
>
> There are three main helper functions introduced in this patch:
>
> - process_qualifier_constant()
>
>  Used to evaluate qualifiers of type ast_expression
>
> - ast_layout_expression::process_qualifier_constant()
>
>  Used to merge and then evaluate qualifiers of type ast_layout_expression
>
> - ast_layout_expression::merge_qualifier()
>
>  Simply appends a qualifier to a list to be merged later by
>  process_qualifier_constant()
>
> In order to avoid cascading error messages the process_qualifier_constant()
> helpers return a bool.
> ---
>  src/glsl/ast.h                  |  60 ++++++---
>  src/glsl/ast_to_hir.cpp         | 274 ++++++++++++++++++++++++++++------------
>  src/glsl/ast_type.cpp           | 128 +++++++++++--------
>  src/glsl/glsl_parser.yy         |  25 ++--
>  src/glsl/glsl_parser_extras.cpp |  68 ++++++----
>  5 files changed, 363 insertions(+), 192 deletions(-)
>
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> index afd2d41..8fbb5dd 100644
> --- a/src/glsl/ast.h
> +++ b/src/glsl/ast.h
> @@ -350,6 +350,26 @@ public:
>     exec_list array_dimensions;
>  };
>
> +class ast_layout_expression : public ast_node {
> +public:
> +   ast_layout_expression(const struct YYLTYPE &locp, ast_expression *expr)
> +   {
> +      set_location(locp);
> +      layout_const_expressions.push_tail(&expr->link);
> +   }
> +
> +   bool process_qualifier_constant(struct _mesa_glsl_parse_state *state,
> +                                   const char *qual_indentifier,
> +                                   int *value);
> +
> +   void merge_qualifier(ast_layout_expression *l_expr)
> +   {
> +      layout_const_expressions.append_list(&l_expr->layout_const_expressions);
> +   }
> +
> +   exec_list layout_const_expressions;
> +};
> +
>  /**
>   * C-style aggregate initialization class
>   *
> @@ -553,13 +573,11 @@ struct ast_type_qualifier {
>        uint64_t i;
>     } flags;
>
> -   struct YYLTYPE *loc;
> -
>     /** Precision of the type (highp/medium/lowp). */
>     unsigned precision:2;
>
>     /** Geometry shader invocations for GL_ARB_gpu_shader5. */
> -   int invocations;
> +   ast_layout_expression *invocations;
>
>     /**
>      * Location specified via GL_ARB_explicit_attrib_location layout
> @@ -567,20 +585,20 @@ struct ast_type_qualifier {
>      * \note
>      * This field is only valid if \c explicit_location is set.
>      */
> -   int location;
> +   ast_expression *location;
>     /**
>      * Index specified via GL_ARB_explicit_attrib_location layout
>      *
>      * \note
>      * This field is only valid if \c explicit_index is set.
>      */
> -   int index;
> +   ast_expression *index;
>
>     /** Maximum output vertices in GLSL 1.50 geometry shaders. */
> -   int max_vertices;
> +   ast_layout_expression *max_vertices;
>
>     /** Stream in GLSL 1.50 geometry shaders. */
> -   unsigned stream;
> +   ast_expression *stream;
>
>     /**
>      * Input or output primitive type in GLSL 1.50 geometry shaders
> @@ -594,7 +612,7 @@ struct ast_type_qualifier {
>      * \note
>      * This field is only valid if \c explicit_binding is set.
>      */
> -   int binding;
> +   ast_expression *binding;
>
>     /**
>      * Offset specified via GL_ARB_shader_atomic_counter's "offset"
> @@ -603,14 +621,14 @@ struct ast_type_qualifier {
>      * \note
>      * This field is only valid if \c explicit_offset is set.
>      */
> -   int offset;
> +   ast_expression *offset;
>
>     /**
>      * Local size specified via GL_ARB_compute_shader's "local_size_{x,y,z}"
>      * layout qualifier.  Element i of this array is only valid if
>      * flags.q.local_size & (1 << i) is set.
>      */
> -   int local_size[3];
> +   ast_layout_expression *local_size[3];
>
>     /** Tessellation evaluation shader: vertex spacing (equal, fractional even/odd) */
>     GLenum vertex_spacing;
> @@ -622,7 +640,7 @@ struct ast_type_qualifier {
>     bool point_mode;
>
>     /** Tessellation control shader: number of output vertices */
> -   int vertices;
> +   ast_layout_expression *vertices;
>
>     /**
>      * Image format specified with an ARB_shader_image_load_store
> @@ -1094,17 +1112,13 @@ public:
>  class ast_tcs_output_layout : public ast_node
>  {
>  public:
> -   ast_tcs_output_layout(const struct YYLTYPE &locp, int vertices)
> -      : vertices(vertices)
> +   ast_tcs_output_layout(const struct YYLTYPE &locp)
>     {
>        set_location(locp);
>     }
>
>     virtual ir_rvalue *hir(exec_list *instructions,
>                            struct _mesa_glsl_parse_state *state);
> -
> -private:
> -   const int vertices;
>  };
>
>
> @@ -1136,9 +1150,10 @@ private:
>  class ast_cs_input_layout : public ast_node
>  {
>  public:
> -   ast_cs_input_layout(const struct YYLTYPE &locp, const unsigned *local_size)
> +   ast_cs_input_layout(const struct YYLTYPE &locp,
> +                       ast_layout_expression **local_size)
>     {
> -      memcpy(this->local_size, local_size, sizeof(this->local_size));
> +      memcpy(this->local_size, *local_size, sizeof(this->local_size));
>        set_location(locp);
>     }
>
> @@ -1146,11 +1161,18 @@ public:
>                            struct _mesa_glsl_parse_state *state);
>
>  private:
> -   unsigned local_size[3];
> +   ast_layout_expression *local_size[3];
>  };
>
>  /*@}*/
>
> +bool
> +process_qualifier_constant(struct _mesa_glsl_parse_state *state,
> +                           YYLTYPE *loc,
> +                           const char *qual_indentifier,
> +                           ast_expression *const_expression,
> +                           int *value);
> +
>  extern void
>  _mesa_ast_to_hir(exec_list *instructions, struct _mesa_glsl_parse_state *state);
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 8fb8875..e71c539 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2261,6 +2261,54 @@ validate_matrix_layout_for_type(struct _mesa_glsl_parse_state *state,
>     }
>  }
>
> +bool
> +process_qualifier_constant(struct _mesa_glsl_parse_state *state,
> +                           YYLTYPE *loc,
> +                           const char *qual_indentifier,
> +                           ast_expression *const_expression,
> +                           int *value)
> +{
> +   exec_list dummy_instructions;
> +
> +   if (const_expression == NULL) {
> +      *value = 0;
> +      return true;
> +   }
> +
> +   ir_rvalue *const ir = const_expression->hir(&dummy_instructions, state);
> +
> +   ir_constant *const const_int = ir->constant_expression_value();
> +   if (const_int == NULL || !const_int->type->is_integer()) {
> +      _mesa_glsl_error(loc, state, "%s must be an integral constant "
> +                       "expression", qual_indentifier);
> +      return false;
> +   }
> +
> +   /* If the location is const (and we've verified that
> +    * it is) then no instructions should have been emitted
> +    * when we converted it to HIR. If they were emitted,
> +    * then either the location isn't const after all, or
> +    * we are emitting unnecessary instructions.
> +    */
> +   assert(dummy_instructions.is_empty());
> +
> +   *value = const_int->value.i[0];
> +   return true;
> +}
> +
> +static void
> +validate_stream_qualifier(YYLTYPE *loc, struct _mesa_glsl_parse_state *state,
> +                          int stream)
> +{
> +   if (stream < 0 ||
> +       (unsigned) stream >= state->ctx->Const.MaxVertexStreams) {
> +      _mesa_glsl_error(loc, state,
> +                       "invalid stream specified %d is larger than "
> +                       "MAX_VERTEX_STREAMS - 1 (%d) or less than 0.",
> +                       stream, state->ctx->Const.MaxVertexStreams - 1);
> +   }
> +}
> +
>  static bool
>  validate_binding_qualifier(struct _mesa_glsl_parse_state *state,
>                             YYLTYPE *loc,
> @@ -2274,14 +2322,20 @@ validate_binding_qualifier(struct _mesa_glsl_parse_state *state,
>        return false;
>     }
>
> -   if (qual->binding < 0) {
> +   int qual_binding;
> +   if (!process_qualifier_constant(state, loc, "binding",
> +                                   qual->binding, &qual_binding)) {
> +      return false;
> +   }
> +
> +   if (qual_binding < 0) {
>        _mesa_glsl_error(loc, state, "binding values must be >= 0");
>        return false;
>     }
>
>     const struct gl_context *const ctx = state->ctx;
>     unsigned elements = type->is_array() ? type->arrays_of_arrays_size() : 1;
> -   unsigned max_index = qual->binding + elements - 1;
> +   unsigned max_index = qual_binding + elements - 1;
>     const glsl_type *base_type = type->without_array();
>
>     if (base_type->is_interface()) {
> @@ -2299,7 +2353,7 @@ validate_binding_qualifier(struct _mesa_glsl_parse_state *state,
>           max_index >= ctx->Const.MaxUniformBufferBindings) {
>           _mesa_glsl_error(loc, state, "layout(binding = %d) for %d UBOs exceeds "
>                            "the maximum number of UBO binding points (%d)",
> -                          qual->binding, elements,
> +                          qual_binding, elements,
>                            ctx->Const.MaxUniformBufferBindings);
>           return false;
>        }
> @@ -2317,7 +2371,7 @@ validate_binding_qualifier(struct _mesa_glsl_parse_state *state,
>           max_index >= ctx->Const.MaxShaderStorageBufferBindings) {
>           _mesa_glsl_error(loc, state, "layout(binding = %d) for %d SSBOs exceeds "
>                            "the maximum number of SSBO binding points (%d)",
> -                          qual->binding, elements,
> +                          qual_binding, elements,
>                            ctx->Const.MaxShaderStorageBufferBindings);
>           return false;
>        }
> @@ -2334,16 +2388,16 @@ validate_binding_qualifier(struct _mesa_glsl_parse_state *state,
>        if (max_index >= limit) {
>           _mesa_glsl_error(loc, state, "layout(binding = %d) for %d samplers "
>                            "exceeds the maximum number of texture image units "
> -                          "(%d)", qual->binding, elements, limit);
> +                          "(%d)", qual_binding, elements, limit);
>
>           return false;
>        }
>     } else if (base_type->contains_atomic()) {
>        assert(ctx->Const.MaxAtomicBufferBindings <= MAX_COMBINED_ATOMIC_BUFFERS);
> -      if (unsigned(qual->binding) >= ctx->Const.MaxAtomicBufferBindings) {
> +      if (unsigned(qual_binding) >= ctx->Const.MaxAtomicBufferBindings) {
>           _mesa_glsl_error(loc, state, "layout(binding = %d) exceeds the "
>                            " maximum number of atomic counter buffer bindings"
> -                          "(%d)", qual->binding,
> +                          "(%d)", qual_binding,
>                            ctx->Const.MaxAtomicBufferBindings);
>
>           return false;
> @@ -2413,10 +2467,16 @@ validate_explicit_location(const struct ast_type_qualifier *qual,
>                             YYLTYPE *loc)
>  {
>     bool fail = false;
> +   int qual_location;
>
> -   if (qual->location < 0) {
> +   if (!process_qualifier_constant(state, loc, "location",
> +                                   qual->location, &qual_location)) {
> +      return;
> +   }
> +
> +   if (qual_location < 0) {
>         _mesa_glsl_error(loc, state, "invalid location %d specified",
> -                        qual->location);
> +                        qual_location);
>         return;
>     }
>
> @@ -2426,7 +2486,7 @@ validate_explicit_location(const struct ast_type_qualifier *qual,
>           return;
>
>        const struct gl_context *const ctx = state->ctx;
> -      unsigned max_loc = qual->location + var->type->uniform_locations() - 1;
> +      unsigned max_loc = qual_location + var->type->uniform_locations() - 1;
>
>        if (max_loc >= ctx->Const.MaxUserAssignableUniformLocations) {
>           _mesa_glsl_error(loc, state, "location(s) consumed by uniform %s "
> @@ -2436,7 +2496,7 @@ validate_explicit_location(const struct ast_type_qualifier *qual,
>        }
>
>        var->data.explicit_location = true;
> -      var->data.location = qual->location;
> +      var->data.location = qual_location;
>        return;
>     }
>
> @@ -2521,23 +2581,23 @@ validate_explicit_location(const struct ast_type_qualifier *qual,
>        switch (state->stage) {
>        case MESA_SHADER_VERTEX:
>           var->data.location = (var->data.mode == ir_var_shader_in)
> -            ? (qual->location + VERT_ATTRIB_GENERIC0)
> -            : (qual->location + VARYING_SLOT_VAR0);
> +            ? (qual_location + VERT_ATTRIB_GENERIC0)
> +            : (qual_location + VARYING_SLOT_VAR0);
>           break;
>
>        case MESA_SHADER_TESS_CTRL:
>        case MESA_SHADER_TESS_EVAL:
>        case MESA_SHADER_GEOMETRY:
>           if (var->data.patch)
> -            var->data.location = qual->location + VARYING_SLOT_PATCH0;
> +            var->data.location = qual_location + VARYING_SLOT_PATCH0;
>           else
> -            var->data.location = qual->location + VARYING_SLOT_VAR0;
> +            var->data.location = qual_location + VARYING_SLOT_VAR0;
>           break;
>
>        case MESA_SHADER_FRAGMENT:
>           var->data.location = (var->data.mode == ir_var_shader_out)
> -            ? (qual->location + FRAG_RESULT_DATA0)
> -            : (qual->location + VARYING_SLOT_VAR0);
> +            ? (qual_location + FRAG_RESULT_DATA0)
> +            : (qual_location + VARYING_SLOT_VAR0);
>           break;
>        case MESA_SHADER_COMPUTE:
>           assert(!"Unexpected shader type");
> @@ -2566,12 +2626,15 @@ validate_layout_qualifiers(const struct ast_type_qualifier *qual,
>            * Older specifications don't mandate a behavior; we take
>            * this as a clarification and always generate the error.
>            */
> -         if (qual->index < 0 || qual->index > 1) {
> +         int qual_index;
> +         if (process_qualifier_constant(state, loc, "index",
> +                                        qual->index, &qual_index) &&
> +             (qual_index < 0 || qual_index > 1)) {
>              _mesa_glsl_error(loc, state,
>                               "explicit index may only be 0 or 1");
>           } else {
>              var->data.explicit_index = true;
> -            var->data.index = qual->index;
> +            var->data.index = qual_index;
Perhaps we should return If process_qualifier_constant fails ?
Otherwise we're assigning garbage to data.index. I'm leaning that we
should modify validate_layout_qualifiers() to return bool and the call
chain up to honour it.

>           }
>        }
>     } else if (qual->flags.q.explicit_index) {
> @@ -2580,8 +2643,11 @@ validate_layout_qualifiers(const struct ast_type_qualifier *qual,
>
>     if (qual->flags.q.explicit_binding &&
>         validate_binding_qualifier(state, loc, var->type, qual)) {
> +      int qual_binding;
> +      process_qualifier_constant(state, loc, "binding",
> +                                 qual->binding, &qual_binding);
process, then validate ? Here and below we just continue if
process/validate fails, as per the original code. I'm not 100% sure if
we don't want to change that.

>        var->data.explicit_binding = true;
> -      var->data.binding = qual->binding;
> +      var->data.binding = qual_binding;
>     }
>
>     if (var->type->contains_atomic()) {
> @@ -2785,7 +2851,12 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>
>     if (state->stage == MESA_SHADER_GEOMETRY &&
>         qual->flags.q.out && qual->flags.q.stream) {
> -      var->data.stream = qual->stream;
> +      int qual_stream;
> +      if (process_qualifier_constant(state, loc, "stream", qual->stream,
> +                                     &qual_stream)) {
> +         validate_stream_qualifier(loc, state, qual_stream);
> +         var->data.stream = qual_stream;
> +      }
>     }
>
>     if (qual->flags.q.patch)
> @@ -3544,10 +3615,14 @@ static void
>  handle_tess_ctrl_shader_output_decl(struct _mesa_glsl_parse_state *state,
>                                      YYLTYPE loc, ir_variable *var)
>  {
> -   unsigned num_vertices = 0;
> +   int num_vertices = 0;
With this change, we should check if the num_vertices doesn't end up
negative. A tweak in validate_layout_qualifier_vertex_count() perhaps
?

>
>     if (state->tcs_output_vertices_specified) {
> -      num_vertices = state->out_qualifier->vertices;
> +      if (!state->out_qualifier->vertices->
> +             process_qualifier_constant(state, "vertices",
> +                                        &num_vertices)) {
> +         return;
> +      }
>     }
>
>     if (!var->type->is_array() && !var->data.patch) {
> @@ -3561,7 +3636,8 @@ handle_tess_ctrl_shader_output_decl(struct _mesa_glsl_parse_state *state,
>     if (var->data.patch)
>        return;
>
> -   validate_layout_qualifier_vertex_count(state, loc, var, num_vertices,
> +   validate_layout_qualifier_vertex_count(state, loc, var,
> +                                          (unsigned) num_vertices,
... and drop the cast ?

>                                            &state->tcs_output_size,
>                                            "tessellation control shader output");
>  }
> @@ -3817,9 +3893,20 @@ 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) {
> +         int qual_binding;
> +         if (process_qualifier_constant(state, &loc, "binding",
> +                                        type->qualifier.binding,
> +                                        &qual_binding)) {
> +            int qual_offset;
> +            if (process_qualifier_constant(state, &loc, "offset",
> +                                           type->qualifier.offset,
> +                                           &qual_offset)) {
> +               state->atomic_counter_offsets[qual_binding] =
> +                  (unsigned) qual_offset;
Both casts look wrong imho. Missing validate ?

> +            }
> +         }
> +      }
>     }
>
>     if (this->declarations.is_empty()) {
> @@ -5980,17 +6067,18 @@ ast_process_structure_or_interface_block(exec_list *instructions,
>           fields[i].sample = qual->flags.q.sample ? 1 : 0;
>           fields[i].patch = qual->flags.q.patch ? 1 : 0;
>
> +         /* Only save explicitly defined streams in block's field */
>           if (qual->flags.q.explicit_stream) {
> -            if (qual->stream < 0) {
> -               YYLTYPE loc = decl_list->get_location();
> -               _mesa_glsl_error(&loc, state, "invalid stream %d specified",
> -                                qual->stream);
> +            int qual_stream;
> +            if (process_qualifier_constant(state, &loc, "stream",
> +                                           qual->stream, &qual_stream)) {
> +               validate_stream_qualifier(&loc, state, qual_stream);
> +               fields[i].stream = qual_stream;
>              }
> +         } else {
> +            fields[i].stream = -1;
>           }
>
> -         /* Only save explicitly defined streams in block's field */
> -         fields[i].stream = qual->flags.q.explicit_stream ? qual->stream : -1;
> -
>           if (qual->flags.q.row_major || qual->flags.q.column_major) {
>              if (!qual->flags.q.uniform && !qual->flags.q.buffer) {
>                 _mesa_glsl_error(&loc, state,
> @@ -6291,15 +6379,19 @@ ast_interface_block::hir(exec_list *instructions,
>
>     state->struct_specifier_depth--;
>
> -   for (unsigned i = 0; i < num_variables; i++) {
> -      if (fields[i].stream != -1 &&
> -          (unsigned) fields[i].stream != this->layout.stream) {
> -         _mesa_glsl_error(&loc, state,
> -                          "stream layout qualifier on "
> -                          "interface block member `%s' does not match "
> -                          "the interface block (%d vs %d)",
> -                          fields[i].name, fields[i].stream,
> -                          this->layout.stream);
> +   int qual_stream;
> +   if (process_qualifier_constant(state, &loc, "stream", this->layout.stream,
> +                                  &qual_stream)) {
> +      validate_stream_qualifier(&loc, state, qual_stream);
Didn't we just do that in ast_process_structure_or_interface_block() -
the call is just above the struct_specifier_depth--;

> +
> +      for (unsigned i = 0; i < num_variables; i++) {
> +         if (fields[i].stream != -1 && fields[i].stream != qual_stream) {
> +            _mesa_glsl_error(&loc, state,
> +                             "stream layout qualifier on "
> +                             "interface block member `%s' does not match "
> +                             "the interface block (%d vs %d)",
> +                             fields[i].name, fields[i].stream, qual_stream);
> +         }
>        }
>     }
>
> @@ -6641,9 +6733,15 @@ ast_interface_block::hir(exec_list *instructions,
>            * has an instance name.  This is ugly.
>            */
>           var->data.explicit_binding = this->layout.flags.q.explicit_binding;
> -         var->data.binding = this->layout.binding;
> +         int qual_binding;
> +         if (this->layout.flags.q.explicit_binding &&
> +             process_qualifier_constant(state, &loc, "binding",
> +                                        this->layout.binding,
> +                                        &qual_binding)) {
> +            var->data.binding = qual_binding;
> +         }
I'm leaning that we should have already processed the binding qualifier.

>
> -         var->data.stream = this->layout.stream;
> +         var->data.stream = qual_stream;
>
>           state->symbols->add_variable(var);
>           instructions->push_tail(var);
> @@ -6663,7 +6761,7 @@ ast_interface_block::hir(exec_list *instructions,
>           var->data.centroid = fields[i].centroid;
>           var->data.sample = fields[i].sample;
>           var->data.patch = fields[i].patch;
> -         var->data.stream = this->layout.stream;
> +         var->data.stream = qual_stream;
>           var->init_interface_type(block_type);
>
>           if (var_mode == ir_var_shader_in || var_mode == ir_var_uniform)
> @@ -6714,7 +6812,13 @@ ast_interface_block::hir(exec_list *instructions,
>            * has an instance name.  This is ugly.
>            */
>           var->data.explicit_binding = this->layout.flags.q.explicit_binding;
> -         var->data.binding = this->layout.binding;
> +         int qual_binding;
> +         if (this->layout.flags.q.explicit_binding &&
> +             process_qualifier_constant(state, &loc, "binding",
> +                                        this->layout.binding,
> +                                        &qual_binding)) {
> +            var->data.binding = qual_binding;
> +         }
Having another blond moment... and I'll stop before I get too annoying.

Just an idea - have you considered converting one qualifier at a time
- it should ease the 450 diffstat (and review)

-Emil


More information about the mesa-dev mailing list