[Mesa-dev] [PATCH 4/6] glsl: add support for complie-time constant expressions
Timothy Arceri
timothy.arceri at collabora.com
Fri Nov 6 14:40:16 PST 2015
On Fri, 2015-11-06 at 14:17 +0000, Emil Velikov wrote:
> Hi Tim,
Hi Emil, Thanks for taking a look over these.
>
> 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 ?
Yeah I was wondering the same thing when I started but I think in most cases
this is fine and in fact desirable. The reason to continue doing validation
even after we have hit an error is because we can have a list of qualifiers
for example:
layout(offset = 44, align = 8)
If offset is invalid we can continue on and check align so that we can return
two error messages if they are both invalid. This is a more useful way to
return error messages then doing it one at a time as the developer fixes their
code.
With compile time constants we need to skip over the rest of the validations
for the current qualifier we are processing if there is an error to avoid
returning incorrect error messages, for example if the expression is not a
constant expression we don't have any value to validate so we don't want to
return incorrect error messages.
>
> 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.
Yeah I thought about this but as there is no further validation on index and
we have set an error I don't think it really matters.
> I'm leaning that we
> should modify validate_layout_qualifiers() to return bool and the call
> chain up to honour it.
As per my reply a the top of the email I don't think this is a good idea.
>
> > }
> > }
> > } 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.
The validate calls process too, its kind of yuck but this is the only place
that calls the validate function to need the constant for further processing
so we really call it twice, seemed nicer to have the process inside the
validate rather than pasting it everywhere else.
The second process should never fail, again I think its a good thing to
continue validating other qualifiers after an error.
>
> > 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
> ?
It's checked in set_shader_inout_layout() but I think you are right it would
be better to move the check here otherwise we would end up doing odd things
like creating huge array types if the value was negative. I think this change
should be made in the previous patch, good catch.
>
> >
> > 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 ?
We would still need this I believe.
>
> > &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 ?
Right. Will add some validation here.
>
> > + }
> > + }
> > + }
> > }
> >
> > 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--;
No that was for members this is the interface itself, however this could and
probably should be moved in there to avoid the extra loop.
Also the above will code will cause an error if a member has a stream set but
the interface doesn't, I looks like the original code did this too I'll need
to check the spec to see if thats correct.
>
> > +
> > + 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.
?? Not for the interface this is the first time its done.
>
> >
> > - 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.
hehe
>
> Just an idea - have you considered converting one qualifier at a time
> - it should ease the 450 diffstat (and review)
I could break it up for review, but it would need to be merged before pushing
as the parser is not this kind on you, there can be only a single syntax
match.
For example we can have:
any_identifier '=' integer_constant
or
any_identifier '=' constant_expression
but not both as constant_expression includes the same rules as
integer_constant
so we need to convert all the qualifiers at once.
>
> -Emil
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list