[Mesa-dev] [PATCH V2 10/12] glsl: add support for complie-time constant expressions
Timothy Arceri
t_arceri at yahoo.com.au
Tue Nov 10 13:12:17 PST 2015
On Tue, 2015-11-10 at 13:43 +0000, Emil Velikov wrote:
> Hi Tim,
>
> Mostly trivial suggestions, and one bug caught :-)
>
> On 8 November 2015 at 22:34, 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
> typo "integer"
> > the new ast_layout_expression type if the qualifier requires merging
> > or ast_expression if the qualifier can't have mulitple declorations
> "multiple declarations"
>
> > or if all but he newest qualifier is simply ignored.
> >
> > This also remove the location field that was temporarily added to
> > ast_type_qualifier to keep track of the parser location.
> > ---
> > src/glsl/ast.h | 33 +++---
> > src/glsl/ast_to_hir.cpp | 253 +++++++++++++++++++++++++----------
> > -----
> > src/glsl/ast_type.cpp | 81 +++++--------
> > src/glsl/glsl_parser.yy | 25 ++--
> > src/glsl/glsl_parser_extras.cpp | 43 ++++---
> > 5 files changed, 236 insertions(+), 199 deletions(-)
> >
> > diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> > index ef94cff..4fd049c 100644
> > --- a/src/glsl/ast.h
> > +++ b/src/glsl/ast.h
>
> > @@ -1156,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));
> While we do use this in other places in mesa it's quite a bit of abuse
> of C++. This will involve setting new[] (template) overrides which use
> ralloc and using them. Obviously not part of this patch, but more of a
> FYI.
Right I didn't feel good about doing this I was just working with the existing
code. I feel the local_size qualifier code might still be able to be
simplified will take another look.
>
>
> > --- a/src/glsl/ast_to_hir.cpp
> > +++ b/src/glsl/ast_to_hir.cpp
>
> > @@ -2467,11 +2463,11 @@ validate_explicit_location(const struct
> > ast_type_qualifier *qual,
> > YYLTYPE *loc)
> > {
> > bool fail = false;
> > + unsigned qual_location;
> >
> > - if (qual->location < 0) {
> > - _mesa_glsl_error(loc, state, "invalid location %d specified",
> > - qual->location);
> > - return;
> > + if (!process_qualifier_constant(state, loc, "location",
> > + qual->location, &qual_location, 0)) {
> > + return;
> Most/all(?) other places have their process() in the caller, rather
> than in validate().
I'd rather leave this inside the validate as this is where the values are
copied to the var and everything is self contained.
>
>
> > @@ -2620,22 +2616,30 @@ 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) {
> > + unsigned qual_index;
> > + if (process_qualifier_constant(state, loc, "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;
> This looks wrong. I recommend splitting out the process and validate
> steps. Yes the latter is trivial yet enough to catch you here.
Will change.
>
> Namely:
> Currently we assign garbage into data.index even if process() fails.
>
>
> > @@ -2839,7 +2843,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) {
> Not 100% sure but I'm leaning that we checking
> has_explicit_attrib_stream(), or flags.q.explicit_stream. Based
> (biased:P) on what other places do. Obviously not meant to be part of
> this patch.
qual->flags.q.stream is the check you want and is already done it's just not
named explicit_stream :)
>
> > - var->data.stream = qual->stream;
> > + unsigned qual_stream;
> > + if (process_qualifier_constant(state, loc, "stream", qual->stream,
> > + &qual_stream, 0)) {
> > + validate_stream_qualifier(loc, state, qual_stream);
> There are various approaches of using the process/validate combo in this
> patch.
>
> Imho it's worth adding a comment when we can ignore skip validate,
> and/or ignore its return value.
In this case it should also be part of the if, I'll fix this thanks.
>
>
> > @@ -3598,15 +3607,16 @@ static void
> > handle_tess_ctrl_shader_output_decl(struct _mesa_glsl_parse_state *state,
> > YYLTYPE loc, ir_variable *var)
> > {
> > - int num_vertices = 0;
> > + unsigned num_vertices = 0;
> >
> > if (state->tcs_output_vertices_specified) {
> > - num_vertices = state->out_qualifier->vertices;
> > - if (num_vertices <= 0) {
> > - _mesa_glsl_error(&loc, state, "invalid vertices (%d) specified",
> > - num_vertices);
> > + if (!state->out_qualifier->vertices->
> > + process_qualifier_constant(state, "vertices",
> > + &num_vertices, 0)) {
> Off by one ? Original code checks for <= 0 while process() will does <
> 0. Although on second thought the num_vertices == 0 || num_vertices >
> MaxFoo is a validation stage, so we might want to keep it below.
I'll fix this in the process_qualifier_constant call thanks for spotting :)
The idea is that every qualifier has a minimum value so the validation is done
in a common location, as well as code reuse this ensures that we don't forget
to do the validation which was happening in some places with the old code.
Thinking about this some more I might be able to move the max validation here
too, after a very quick look I've already spotted some places where checking
the max values is not done when the spec says it should be.
>
> > return;
> > - } else if ((unsigned) num_vertices > state->Const.MaxPatchVertices)
> > {
> > + }
> > +
> > + if (num_vertices > state->Const.MaxPatchVertices) {
> > _mesa_glsl_error(&loc, state, "vertices (%d) exceeds "
> > "GL_MAX_PATCH_VERTICES", num_vertices);
> > return;
>
> > @@ -3881,9 +3890,19 @@ 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;
> > + if (process_qualifier_constant(state, &loc, "binding",
> > + type->qualifier.binding,
> > + &qual_binding, 0)) {
> > + unsigned qual_offset;
> > + if (process_qualifier_constant(state, &loc, "offset",
> > + type->qualifier.offset,
> > + &qual_offset, 0)) {
> Bikeshed: declare both variables and combine the process(binding) &&
> process(offset) into a single if ?
Sure.
>
>
> > @@ -6011,8 +6040,12 @@ ast_process_structure_or_interface_block(exec_list
> > *instructions,
> > const struct ast_type_qualifier *const qual =
> > & decl_list->type->qualifier;
> >
> > - if (qual->flags.q.explicit_binding)
> > - validate_binding_qualifier(state, &loc, decl_type, qual);
> > + unsigned qual_binding;
> > + if (qual->flags.q.explicit_binding &&
> > + process_qualifier_constant(state, &loc, "binding",
> > + qual->binding, &qual_binding, 0))
> Bikeshed: might want to keep the following pattern, or not as long as
> it's consistent throughout :)
>
> if (flag)
> unsigned qual_foo
> if (process(foo))
> validate(foo)
Sure.
>
>
> > + validate_binding_qualifier(state, &loc, decl_type,
> > + qual, qual_binding);
> >
> > if (qual->flags.q.std140 ||
> > qual->flags.q.std430 ||
> > @@ -6050,12 +6083,17 @@ ast_process_structure_or_interface_block(exec_list
> > *instructions,
> > * the specified stream must match the stream associated with
> > the
> > * containing block."
> > */
> > - if (qual->flags.q.explicit_stream &&
> > - qual->stream != 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, qual->stream, layout
> > ->stream);
> > + if (qual->flags.q.explicit_stream && block_stream_processed) {
> > + unsigned qual_stream;
> > + if (process_qualifier_constant(state, &loc, "stream",
> > + qual->stream, &qual_stream,
> > 0)) {
> > + if (qual_stream != *block_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, qual_stream,
> > *block_stream);
> > + }
> > + }
> Was going to say "hey
> ast_layout_expression::process_qualifier_constant() already does that
> for you" only to realise that it deals with a linked list, while we
> have an array. Shame we cannot reuse it :(
Yeah ast_layout_expression::process_qualifier_constant() is for the case were
we have.
layout(stream = 1,stream = 1,stream = 1,stream = 1, ... ,stream = 1) var;
Which is legal :(
Were this is for
layout(stream = 1) block {
layout(stream = 1) var;
layout(stream = 1) var;
layout(stream = 1) var;
...
layout(stream = 1) var;
}
More information about the mesa-dev
mailing list