[Mesa-dev] [PATCH V2 07/12] glsl: add new type from compile time constants

[Timothy Arceri]

On Tue, 2015-11-10 at 12:33 +0000, Emil Velikov wrote:
> Hi Tim,
> 
> On 8 November 2015 at 22:34, 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 so we need to store them to be proccessed later.
> > 
> > The new type has two helper functions:
> > 
> > - process_qualifier_constant()
> > 
> >  Used to merge and then evaluate qualifier expressions
> > 
> > - 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        | 20 ++++++++++++++++++
> >  src/glsl/ast_type.cpp | 57
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 77 insertions(+)
> > 
> > diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> > index afd2d41..ef94cff 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,
> > +                                   unsigned *value, int min_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
> >   *
> > diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
> > index 53d1023..8ceb3b1 100644
> > --- a/src/glsl/ast_type.cpp
> > +++ b/src/glsl/ast_type.cpp
> > @@ -482,3 +482,60 @@ ast_type_qualifier::merge_in_qualifier(YYLTYPE *loc,
> > 
> >     return true;
> >  }
> > +
> > +bool
> > +ast_layout_expression::process_qualifier_constant(struct
> > _mesa_glsl_parse_state *state,
> > +                                                  const char
> > *qual_indentifier,
> > +                                                  unsigned *value,
> > +                                                  int min_value)
> > +{
> > +   bool first_pass = true;
> > +   *value = 0;
> > +
> > +   for (exec_node *node = layout_const_expressions.head;
> > +           !node->is_tail_sentinel(); node = node->next) {
> > +
> > +      exec_list dummy_instructions;
> > +      ast_node *const_expression = exec_node_data(ast_node, node, link);
> > +
> > +      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()) {
> > +         YYLTYPE loc = const_expression->get_location();
> > +         _mesa_glsl_error(&loc, state, "%s must be an integral constant "
> > +                          "expression", qual_indentifier);
> > +         return false;
> Based on your earlier comment - "parse and print as much information
> (errors) to the developer, as possible" we can just keep track of the
> failure (same goes below) and just return false once all the
> processing is complete ?
> 
> Not a big deal either way, mostly my internal voice that shouts
> "consistence!" than anything else.

The previous comments about errors back to the developer applied to different
qualifiers on the same variable.

Its ok to stop validation on a single qualifier if it fails somewhere, we just
want to be able to continue on validating the other qualifiers.

e.g.

layout(location = -1, component = -1)

There are two qualifiers here that we can validate, so were don't want to stop
just because we hit the error in location we want to validate component too.
But there is no point validating each qualifier passed the point where we
detect the error.

Does that make sense?


> 
> Emil