On 21 January 2014 04:19, Timothy Arceri <t_arc...@yahoo.com.au> wrote:
> Signed-off-by: Timothy Arceri <t_arc...@yahoo.com.au>
> ---
> src/glsl/ast.h | 44 +++++++----
> src/glsl/ast_array_index.cpp | 13 ++++
> src/glsl/ast_to_hir.cpp | 160
> +++++++++++++++++++++++++++-------------
> src/glsl/ast_type.cpp | 8 +-
> src/glsl/glsl_parser_extras.cpp | 30 ++++----
> src/glsl/glsl_parser_extras.h | 2 +
> 6 files changed, 167 insertions(+), 90 deletions(-)
>
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> index 76911f0..4dda32e 100644
> --- a/src/glsl/ast.h
> +++ b/src/glsl/ast.h
> @@ -276,6 +276,21 @@ private:
> bool cons;
> };
>
> +class ast_array_specifier : public ast_node {
> +public:
> + ast_array_specifier()
> + : ast_node()
>
Note: it's not necessary to explicitly mention the base class in the
initializer list when its constructor takes no arguments.
> + {
> + dimension_count = 1;
> + }
>
It would be nice to have the constructor(s) of ast_array_specifier
initialize it to a consistent state. As written, it creates an
ast_array_specifier with a dimension count of 1, but with is_unsized_array
set to false and an empty array_dimensions list, which isn't sensible.
I'd recommend having two constructors:
/** Unsized array specifier ([]) */
explicit ast_array_specifier(const struct YYLTYPE &locp)
: dimension_count(1), is_unsized_array(true)
{
set_location(locp);
}
and one for a sized array:
/** Sized array specifier ([dim]) */
ast_array_specifier(ast_expression *dim)
: dimension_count(1), is_unsized_array(false)
{
set_location(locp);
array_dimensions.push_tail(&dim->link);
}
This would have the added advantage of making it easier for a reader of the
class definition to understand how the data structure works.
It would also be nice to have a method which adds a dimension to the array
specifier and maintains the proper invariant on dimension_count:
void add_dimension(ast_expression *dim)
{
array_dimensions.push_tail(&dim->link);
dimension_count++;
}
That way all the logic for maintaining internal consistency of the data
structure is here inside the class definition, rather than in the parser
where it's harder to verify that it's correct.
>
> +
> + virtual void print(void) const;
> +
> + unsigned dimension_count;
>
It would be nice to have a comment above dimension_count explaining that
this includes both sized and unsized dimensions.
> + bool is_unsized_array;
>
It would be nice to have a comment above is_unsized_array explaining that
if true, this means that the array has an unsized outermost dimension.
> + exec_list array_dimensions;
>
It would be nice to have a comment explaining that this list contains
objects of type ast_node containing the sized dimensions only, in
outermost-to-innermost order.
> +};
>
+
> /**
> * C-style aggregate initialization class
> *
> @@ -325,14 +340,15 @@ public:
>
> class ast_declaration : public ast_node {
> public:
> - ast_declaration(const char *identifier, bool is_array, ast_expression
> *array_size,
> - ast_expression *initializer);
> + ast_declaration(const char *identifier, bool is_array,
> + ast_array_specifier *array_specifier,
> + ast_expression *initializer);
> virtual void print(void) const;
>
> const char *identifier;
>
> bool is_array;
> - ast_expression *array_size;
> + ast_array_specifier *array_specifier;
>
Previously the reason we needed is_array was because we used array_size ==
NULL to represent both non-arrays and unsized arrays. Now that we use a
non-NULL array_specifier to represent an unsized array, is_array is
redundant--it should be true exactly when array_specifier != NULL. To
avoid redunancy, I think we should drop is_array from this class entirely.
The same goes for the is_array fields of ast_type_specifier,
ast_parameter_declarator, and ast_interface_block.
If you wanted to remove all of that in a follow up patch, that would be
fine by me.
>
> ast_expression *initializer;
> };
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 4cc8eb1..326aa58 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -1771,63 +1771,116 @@ ast_compound_statement::hir(exec_list
> *instructions,
> return NULL;
> }
>
> +static const unsigned
> +process_array_size(exec_node *node,
> + struct _mesa_glsl_parse_state *state)
>
Since it's hard to infer what this function does from its type signature,
I'd recommend adding a comment above it saying something like "Evaluate the
given exec_node (which should be an ast_node representing a single array
dimension) and return its integer value."
> +{
> + int result = 0;
>
This should have type "unsigned" for consistency with the function
signature and the use of "size->value.u[0]" below.
> + exec_list dummy_instructions;
> +
> + ast_node *array_size = exec_node_data(ast_node, node, link);
> + ir_rvalue *const ir = array_size->hir(& dummy_instructions,
> + state);
> + YYLTYPE loc = array_size->get_location();
> +
> + if (ir != NULL) {
> + if (!ir->type->is_integer()) {
> + _mesa_glsl_error(& loc, state,
> + "array size must be integer type");
> + } else if (!ir->type->is_scalar()) {
> + _mesa_glsl_error(& loc, state,
> + "array size must be scalar type");
> + } else {
> + ir_constant *const size = ir->constant_expression_value();
> +
> + if (size == NULL) {
> + _mesa_glsl_error(& loc, state, "array size must be a "
> + "constant valued expression");
> + } else if (size->value.i[0] <= 0) {
> + _mesa_glsl_error(& loc, state, "array size must be > 0");
> + } else {
> + assert(size->type == ir->type);
> + result = size->value.u[0];
> +
> + /* If the array size 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 array size isn't const after all, or
> + * we are emitting unnecessary instructions.
> + */
> + assert(dummy_instructions.is_empty());
> + }
> + }
> + }
> + return result;
>
Nit pick: it's a bit difficult to follow the nesting of error condition
checking here. You might want to consider doing something like this
instead:
if (ir == NULL) {
_mesa_glsl_error(...);
return 0;
}
if (!ir->type->is_integer()) {
_mesa_glsl_error(...);
return 0;
}
if (!ir->type->is_scalar()) {
_mesa_glsl_error(...);
return 0;
}
ir_constant * const size = ...;
if (size == NULL) {
_mesa_glsl_error(...);
return 0;
}
if (size->value.i[0] <= 0) {
_mesa_glsl_error(...);
return 0;
}
assert(size->type == ir->type);
/* ...no instructions should have been emitted... */
assert(dummy_instructions.is_empty());
return size->value.u[0];
> +}
>
> static const glsl_type *
> -process_array_type(YYLTYPE *loc, const glsl_type *base, ast_node
> *array_size,
> - struct _mesa_glsl_parse_state *state)
> +process_array_type(YYLTYPE *loc, const glsl_type *base,
> + ast_array_specifier *array_specifier,
> + struct _mesa_glsl_parse_state *state)
> {
> - unsigned length = 0;
> + const glsl_type *array_type = NULL;
> + const glsl_type *element_type = base;
> + const glsl_type *array_type_temp = element_type;
>
> if (base == NULL)
> return glsl_type::error_type;
>
I think the only way base could ever be NULL here is if there is a bug
elsewhere in Mesa--does that seem correct to you? IMHO it would be fine to
drop the NULL check entirely. If you really want to be careful, I would
recommend adding an assertion so that it's clear that this is not a
situation that's expected to occur, and so that the bug will be easier to
track down if it ever does:
if (base == NULL) {
assert(!"NULL unexpectedly passed to process_array_type()");
return glsl_type::error_type;
}
>
> - /* From page 19 (page 25) of the GLSL 1.20 spec:
> - *
> - * "Only one-dimensional arrays may be declared."
> - */
> if (base->is_array()) {
> - _mesa_glsl_error(loc, state,
> - "invalid array of `%s' (only one-dimensional arrays
> "
> - "may be declared)",
> - base->name);
> - return glsl_type::error_type;
> +
> + /* From page 19 (page 25) of the GLSL 1.20 spec:
> + *
> + * "Only one-dimensional arrays may be declared."
> + */
> + if (!state->ARB_arrays_of_arrays_enable) {
> + _mesa_glsl_error(loc, state,
> + "invalid array of `%s'"
> + "#version 120 / GL_ARB_arrays_of_arrays "
> + "required for defining arrays of arrays",
>
As in patch 2, I think we should drop mention of version 120 from the error
message.
> + base->name);
> + return glsl_type::error_type;
> + }
> +
> + if (base->length == 0) {
> + _mesa_glsl_error(loc, state,
> + "only the outermost array dimension can "
> + "be unsized",
> + base->name);
> + return glsl_type::error_type;
> + }
> +
> + /* Settings this variable will cause
> + * any array dimensions processed with the base type to
> + * be appended onto the end of the array
> + */
>
+ element_type = base->element_type();
>
This comment is misleading. The only thing element_type is used for later
in the function is to decide whether to set array_type to NULL, which in
turn determines whether we return array_type or error_type. And for
reasons I hope to clarify below, I don't think we need that logic anyhow.
I'd prefer to just drop the element_type variable entirely.
> }
>
> - if (array_size != NULL) {
> - exec_list dummy_instructions;
> - ir_rvalue *const ir = array_size->hir(& dummy_instructions, state);
> - YYLTYPE loc = array_size->get_location();
> + if (array_specifier != NULL) {
>
AFAICT from looking at the callers, this function will never be called with
a NULL value of array_specifier. I would recommend dropping this if-test
to make the function easier to follow.
If you really want to be careful you could always add "|| array_specifier
== NULL" to the assertion check I mentioned earlier. But IMHO it's not
necessary.
>
> - if (ir != NULL) {
> - if (!ir->type->is_integer()) {
> - _mesa_glsl_error(& loc, state, "array size must be integer
> type");
> - } else if (!ir->type->is_scalar()) {
> - _mesa_glsl_error(& loc, state, "array size must be scalar
> type");
> - } else {
> - ir_constant *const size = ir->constant_expression_value();
> -
> - if (size == NULL) {
> - _mesa_glsl_error(& loc, state, "array size must be a "
> - "constant valued expression");
> - } else if (size->value.i[0] <= 0) {
> - _mesa_glsl_error(& loc, state, "array size must be > 0");
> - } else {
> - assert(size->type == ir->type);
> - length = size->value.u[0];
> -
> - /* If the array size 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 array size isn't const after all, or
> - * we are emitting unnecessary instructions.
> - */
> - assert(dummy_instructions.is_empty());
> - }
> - }
> + exec_node *node = array_specifier->array_dimensions.tail_pred;
> +
> + unsigned array_size;
>
Since array_size is only used inside the loop, let's declare it where it's
initialized.
> + for (/* nothing */; !node->is_head_sentinel(); node = node->prev) {
>
Since node is only used inside the loop, we can declare it in the for
statement, like this:
for (exec_node *node = array_specifier->array_dimensions.tail_pred; ...)
> + array_size = process_array_size(node, state);
> + array_type_temp = glsl_type::get_array_instance(array_type_temp,
> + array_size);
> + }
> +
> + if (array_specifier->is_unsized_array) {
> + array_type_temp = glsl_type::get_array_instance(array_type_temp,
> + 0);
> + }
> +
> + if (array_type_temp == element_type) {
> + /* we found no array sizes */
> + array_type = NULL;
> + } else {
> + array_type = array_type_temp;
> }
>
Since array_specifier is never NULL, and ast_array_specifiers only have an
empty array_dimensions list when is_unsized_array is true, array_temp will
never equal element_type when we get to this code. I think we should drop
this check, and the logic in the return statement below, and simply return
array_type_temp. If we do that, we should also be able to drop array_type
and then rename array_type_temp to array_type.
> }
>
> - const glsl_type *array_type = glsl_type::get_array_instance(base,
> length);
> return array_type != NULL ? array_type : glsl_type::error_type;
> }
>
With all those changes I think we can simplify the function down to this:
{
if (base->is_array()) {
/* From page 19 (page 25) of the GLSL 1.20 spec:
*
* "Only one-dimensional arrays may be declared."
*/
if (!state->ARB_arrays_of_arrays_enable) {
_mesa_glsl_error(loc, state,
"invalid array of `%s'"
"GL_ARB_arrays_of_arrays "
"required for defining arrays of arrays",
base->name);
return glsl_type::error_type;
}
if (base->length == 0) {
_mesa_glsl_error(loc, state,
"only the outermost array dimension can "
"be unsized",
base->name);
return glsl_type::error_type;
}
}
const glsl_type *array_type = base;
for (exec_node *node = array_specifier->array_dimensions.tail_pred;
!node->is_head_sentinel(); node = node->prev) {
unsigned array_size = process_array_size(node, state);
array_type = glsl_type::get_array_instance(array_type, array_size);
}
if (array_specifier->is_unsized_array)
array_type = glsl_type::get_array_instance(array_type, 0);
return array_type;
}
Here's one other possible simplifying idea: the callers of
process_array_type() have logic that either looks like this or could be
modified to look like this with suitable refactoring:
if (this->is_array)
type = process_array_type(&loc, type, this->array_specifier, state);
If you take my suggestions above to drop is_array, then that will change to:
if (this->array_specifier != NULL)
type = process_array_type(&loc, type, this->array_specifier, state);
At which point we could modify process_array_type() to simply return base
in the case where array_specifier is NULL. If we do that, the call site
simplifies to just:
type = process_array_type(&loc, type, this->array_specifier, state);
>
> @@ -5038,7 +5095,7 @@ ast_interface_block::hir(exec_list *instructions,
> * interface array size *doesn't* need to be specified is on a
> * geometry shader input.
> */
> - if (this->array_size == NULL &&
> + if (block_array_type->is_unsized_array() &&
>
An alternative that wouldn't require moving the declaration of
block_array_type would be to change this to:
if (this->array_specifier->is_unsized_array && ...
> (state->stage != MESA_SHADER_GEOMETRY || !this->
> layout.flags.q.in)) {
> _mesa_glsl_error(&loc, state,
> "only geometry shader inputs may be unsized "
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
