[PATCH vkd3d] vkd3d-shader/hlsl: Support array initializers.
Giovanni Mascellani
gmascellani at codeweavers.com
Wed Jan 5 08:29:28 CST 2022
Hi,
your patch seems mostly ok to me. The idea of recursively walk the
structure of the to-be-initialized variable and filling its "slots"
popping values from the initializer seems fine to me.
As a minor point, notice that we usually add a space after commas and
after "if"s.
I'd echo Zeb's request to see more tests on this. For example, this
monster of ugliness turns out to be accepted by the native compiler:
---
uniform float4 y;
float4 main() : SV_TARGET
{
float x[6] = {1, 2, 3, 4, 5, y[0]};
struct
{
float a;
int b;
} y = {-1, -2};
float4 array_loc[2][4] = {
11, 12, 13, 14,
21, 22, 23, 24,
31, 32, {{{33}}, 34,
41, 42, {43}, 44,
51, 52, 53, x},
y,
float4(100, 101, 102, 103),
81,
};
return array_loc[1][2];
}
---
This hints the the native compiler probably fully unpacks and flattens
everything on the right hand side before feeding it to something similar
to the algorithm you wrote (in other words, the "boundaries" between
packed values like structures, arrays or non-scalars numerics are
ignored inside the initializer). It is not necessary to immediately
write the most generic algorithm possible, but it helps to already know
what we are targeting.
Giovanni.
On 29/12/21 15:51, Francisco Casas wrote:
> The new function initialize_var_recursive() "pops" arguments from the parse_variable_def
> initializer to initialize the members of var for both struct and array initialization recursively.
> initializer_i is used to keep track of the index of the next input parameter to read.
>
> This approach should scale well for nested arrays and structs.
>
> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
>
> ---
>
> The new definition of free_parse_variable_def() is unorthodox, but I
> think it helps illustrate how the parse_variable_def memory should be
> free on the different scenarios.
> The exercise of writting it this way allowed to find a memory leak.
>
> Ignoring inner braces on initializers is still pending, this could be
> achieved through a flattening method or at the parser level.
>
> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
> ---
> Makefile.am | 4 +
> libs/vkd3d-shader/hlsl.y | 207 +++++++++---------
> .../hlsl-array-initializer-local.shader_test | 21 ++
> .../hlsl-array-initializer-static.shader_test | 21 ++
> 4 files changed, 155 insertions(+), 98 deletions(-)
> create mode 100644 tests/hlsl-array-initializer-local.shader_test
> create mode 100644 tests/hlsl-array-initializer-static.shader_test
>
> diff --git a/Makefile.am b/Makefile.am
> index 20fee06d..91e396ee 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -61,6 +61,8 @@ vkd3d_shader_tests = \
> tests/cast-to-uint.shader_test \
> tests/conditional.shader_test \
> tests/hlsl-array-dimension.shader_test \
> + tests/hlsl-array-initializer-local.shader_test \
> + tests/hlsl-array-initializer-static.shader_test \
> tests/hlsl-bool-cast.shader_test \
> tests/hlsl-clamp.shader_test \
> tests/hlsl-comma.shader_test \
> @@ -289,6 +291,8 @@ XFAIL_TESTS = \
> tests/cast-to-uint.shader_test \
> tests/conditional.shader_test \
> tests/hlsl-array-dimension.shader_test \
> + tests/hlsl-array-initializer-local.shader_test \
> + tests/hlsl-array-initializer-static.shader_test \
> tests/hlsl-bool-cast.shader_test \
> tests/hlsl-duplicate-modifiers.shader_test \
> tests/hlsl-for.shader_test \
> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> index 636882c4..52c32878 100644
> --- a/libs/vkd3d-shader/hlsl.y
> +++ b/libs/vkd3d-shader/hlsl.y
> @@ -904,7 +904,8 @@ static bool expr_compatible_data_types(struct hlsl_type *t1, struct hlsl_type *t
>
> static enum hlsl_base_type expr_common_base_type(enum hlsl_base_type t1, enum hlsl_base_type t2)
> {
> - if (t1 > HLSL_TYPE_LAST_SCALAR || t2 > HLSL_TYPE_LAST_SCALAR) {
> + if (t1 > HLSL_TYPE_LAST_SCALAR || t2 > HLSL_TYPE_LAST_SCALAR)
> + {
> FIXME("Unexpected base type.\n");
> return HLSL_TYPE_FLOAT;
> }
> @@ -1278,71 +1279,113 @@ static bool add_increment(struct hlsl_ctx *ctx, struct list *instrs, bool decrem
> return true;
> }
>
> -static void struct_var_initializer(struct hlsl_ctx *ctx, struct list *list, struct hlsl_ir_var *var,
> - struct parse_initializer *initializer)
> +static void initialize_var_recursive(struct hlsl_ctx *ctx, struct hlsl_ir_var *var, unsigned int reg_offset,
> + struct hlsl_type *type, struct parse_variable_def *v, unsigned int *initializer_i, struct list *instrs)
> {
> - struct hlsl_type *type = var->data_type;
> - struct hlsl_struct_field *field;
> - unsigned int i = 0;
> -
> - if (initializer_size(initializer) != hlsl_type_component_count(type))
> + if (type->type <= HLSL_CLASS_LAST_NUMERIC)
> {
> - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
> - "Expected %u components in initializer, but got %u.",
> - hlsl_type_component_count(type), initializer_size(initializer));
> - free_parse_initializer(initializer);
> - return;
> - }
> + unsigned int writemask_offset = 0;
> + unsigned int components_read = 0;
>
> - list_move_tail(list, initializer->instrs);
> - vkd3d_free(initializer->instrs);
> + if (type->type == HLSL_CLASS_MATRIX)
> + hlsl_fixme(ctx, &var->loc, "Matrix initializer\n");
>
> - LIST_FOR_EACH_ENTRY(field, type->e.elements, struct hlsl_struct_field, entry)
> - {
> - struct hlsl_ir_node *node = initializer->args[i];
> - struct hlsl_ir_store *store;
> - struct hlsl_ir_constant *c;
> + while (components_read < hlsl_type_component_count(type))
> + {
> + struct hlsl_ir_store *store;
> + struct hlsl_ir_constant *c;
> + struct hlsl_ir_node *node;
> + unsigned int width;
>
> - if (i++ >= initializer->args_count)
> - break;
> + assert(*initializer_i < v->initializer.args_count);
> + node = v->initializer.args[*initializer_i];
> + *initializer_i += 1;
>
> - if (hlsl_type_component_count(field->type) == hlsl_type_component_count(node->data_type))
> - {
> - if (!(c = hlsl_new_uint_constant(ctx, field->reg_offset, node->loc)))
> - break;
> - list_add_tail(list, &c->node.entry);
> + width = hlsl_type_component_count(node->data_type);
> + components_read += width;
>
> - if (!(store = hlsl_new_store(ctx, var, &c->node, node, 0, node->loc)))
> - break;
> - list_add_tail(list, &store->node.entry);
> + if (components_read > hlsl_type_component_count(type))
> + {
> + hlsl_error(ctx, &node->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
> + "Initializer argument has spare components.\n");
> + return;
> + }
> +
> + if (!(node = add_implicit_conversion(ctx, instrs, node,
> + hlsl_get_vector_type(ctx, type->base_type, width), &node->loc)))
> + return;
> +
> + if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc)))
> + return;
> + list_add_tail(instrs, &c->node.entry);
> +
> + if (!(store = hlsl_new_store(ctx, var, &c->node, node,
> + ((1u << width) - 1) << writemask_offset, node->loc)))
> + return;
> + list_add_tail(instrs, &store->node.entry);
> +
> + writemask_offset += width;
> }
> - else
> + }
> + else if (type->type == HLSL_CLASS_STRUCT)
> + {
> + struct hlsl_struct_field *field;
> +
> + LIST_FOR_EACH_ENTRY(field, type->e.elements, struct hlsl_struct_field, entry)
> {
> - hlsl_fixme(ctx, &node->loc, "Implicit cast in structure initializer.");
> + initialize_var_recursive(ctx, var, reg_offset + field->reg_offset, field->type, v,
> + initializer_i, instrs);
> }
> + return;
> }
> + else if (type->type == HLSL_CLASS_ARRAY)
> + {
> + for (int i = 0; i < type->e.array.elements_count; i++)
> + {
> + struct hlsl_type *elem_type = type->e.array.type;
> + unsigned int elem_offset = i * elem_type->reg_size;
>
> - vkd3d_free(initializer->args);
> + initialize_var_recursive(ctx, var, reg_offset + elem_offset, elem_type, v,
> + initializer_i, instrs);
> + }
> + return;
> + }
> + else if (type->type == HLSL_CLASS_OBJECT)
> + {
> + hlsl_fixme(ctx, &var->loc, "Initializers for objects not supported yet.\n");
> + return;
> + }
> }
>
> -static void free_parse_variable_def(struct parse_variable_def *v)
> +static void free_parse_variable_def(struct parse_variable_def *v, int freeness)
> {
> - free_parse_initializer(&v->initializer);
> - vkd3d_free(v->arrays.sizes);
> - vkd3d_free(v->name);
> - vkd3d_free((void *)v->semantic.name);
> - vkd3d_free(v);
> + /* 0 : If v's names and instructions were passed to another struct. */
> + /* 1 : If only v's names were passed to another struct. */
> + /* 2 : If v still retains the ownership of all its members. */
> + if (freeness == 0)
> + {
> + vkd3d_free(v->initializer.args);
> + vkd3d_free(v->initializer.instrs);
> + }
> + if (freeness == 1)
> + {
> + free_parse_initializer(&v->initializer);
> + }
> + if (freeness == 2)
> + {
> + free_parse_initializer(&v->initializer);
> + vkd3d_free(v->name);
> + vkd3d_free((void *)v->semantic.name);
> + }
> + vkd3d_free(v->arrays.sizes);
> + vkd3d_free(v);
> }
>
> static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_type,
> DWORD modifiers, struct list *var_list)
> {
> struct parse_variable_def *v, *v_next;
> - struct hlsl_ir_function_decl *func;
> struct list *statements_list;
> - struct hlsl_ir_var *var;
> - struct hlsl_type *type;
> - bool local = true;
>
> if (basic_type->type == HLSL_CLASS_MATRIX)
> assert(basic_type->modifiers & HLSL_MODIFIERS_MAJORITY_MASK);
> @@ -1350,7 +1393,7 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
> if (!(statements_list = make_empty_list(ctx)))
> {
> LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry)
> - free_parse_variable_def(v);
> + free_parse_variable_def(v,2);
> vkd3d_free(var_list);
> return NULL;
> }
> @@ -1360,9 +1403,12 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
>
> LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry)
> {
> + struct hlsl_type *type = basic_type;
> + struct hlsl_ir_function_decl *func;
> + struct hlsl_ir_var *var;
> + bool local = true;
> unsigned int i;
>
> - type = basic_type;
> for (i = 0; i < v->arrays.count; ++i)
> type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]);
>
> @@ -1371,7 +1417,7 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
>
> if (!(var = hlsl_new_var(ctx, v->name, type, v->loc, &v->semantic, modifiers, &v->reg_reservation)))
> {
> - free_parse_variable_def(v);
> + free_parse_variable_def(v,2);
> continue;
> }
>
> @@ -1424,7 +1470,7 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
> hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER,
> "Const variable \"%s\" is missing an initializer.", var->name);
> hlsl_free_var(var);
> - vkd3d_free(v);
> + free_parse_variable_def(v,1);
> continue;
> }
>
> @@ -1436,78 +1482,43 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
> "Variable \"%s\" was already declared in this scope.", var->name);
> hlsl_note(ctx, &old->loc, VKD3D_SHADER_LOG_ERROR, "\"%s\" was previously declared here.", old->name);
> hlsl_free_var(var);
> - vkd3d_free(v);
> + free_parse_variable_def(v,1);
> continue;
> }
>
> if (v->initializer.args_count)
> {
> unsigned int size = initializer_size(&v->initializer);
> - struct hlsl_ir_load *load;
>
> - if (type->type <= HLSL_CLASS_LAST_NUMERIC
> - && type->dimx * type->dimy != size && size != 1)
> - {
> - if (size < type->dimx * type->dimy)
> - {
> - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
> - "Expected %u components in numeric initializer, but got %u.",
> - type->dimx * type->dimy, size);
> - free_parse_initializer(&v->initializer);
> - vkd3d_free(v);
> - continue;
> - }
> - }
> - if ((type->type == HLSL_CLASS_STRUCT || type->type == HLSL_CLASS_ARRAY)
> - && hlsl_type_component_count(type) != size)
> + if (hlsl_type_component_count(type) != size && (size != 1 || type->type > HLSL_CLASS_LAST_NUMERIC))
> {
> hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
> "Expected %u components in initializer, but got %u.", hlsl_type_component_count(type), size);
> - free_parse_initializer(&v->initializer);
> - vkd3d_free(v);
> + free_parse_variable_def(v,1);
> continue;
> }
>
> - if (type->type == HLSL_CLASS_STRUCT)
> + if(var->data_type->type <= HLSL_CLASS_LAST_NUMERIC)
> {
> - struct_var_initializer(ctx, statements_list, var, &v->initializer);
> - vkd3d_free(v);
> - continue;
> - }
> - if (type->type > HLSL_CLASS_LAST_NUMERIC)
> - {
> - FIXME("Initializers for non scalar/struct variables not supported yet.\n");
> - free_parse_initializer(&v->initializer);
> - vkd3d_free(v);
> - continue;
> - }
> - if (v->arrays.count)
> - {
> - hlsl_fixme(ctx, &v->loc, "Array initializer.");
> - free_parse_initializer(&v->initializer);
> - vkd3d_free(v);
> - continue;
> + struct hlsl_ir_load *load;
> +
> + load = hlsl_new_var_load(ctx, var, var->loc);
> + list_add_tail(v->initializer.instrs, &load->node.entry);
> + add_assignment(ctx, v->initializer.instrs, &load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]);
Why can't this be handled by initialize_var_recursive() as the general case?
> }
> - if (v->initializer.args_count > 1)
> + else
> {
> - hlsl_fixme(ctx, &v->loc, "Complex initializer.");
> - free_parse_initializer(&v->initializer);
> - vkd3d_free(v);
> - continue;
> - }
> -
> - load = hlsl_new_var_load(ctx, var, var->loc);
> - list_add_tail(v->initializer.instrs, &load->node.entry);
> - add_assignment(ctx, v->initializer.instrs, &load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]);
> - vkd3d_free(v->initializer.args);
> + unsigned int initializer_i = 0;
>
> + initialize_var_recursive(ctx, var, 0, var->data_type, v, &initializer_i, v->initializer.instrs);
> + }
> if (modifiers & HLSL_STORAGE_STATIC)
> list_move_tail(&ctx->static_initializers, v->initializer.instrs);
> else
> list_move_tail(statements_list, v->initializer.instrs);
> - vkd3d_free(v->initializer.instrs);
> }
> - vkd3d_free(v);
> +
> + free_parse_variable_def(v,0);
> }
> vkd3d_free(var_list);
> return statements_list;
> diff --git a/tests/hlsl-array-initializer-local.shader_test b/tests/hlsl-array-initializer-local.shader_test
> new file mode 100644
> index 00000000..e9310b98
> --- /dev/null
> +++ b/tests/hlsl-array-initializer-local.shader_test
> @@ -0,0 +1,21 @@
> +[pixel shader]
> +float4 main() : SV_TARGET
> +{
> + float4 array_loc[2][4] = {
> + 11, 12, 13, 14,
> + 21, 22, 23, 24,
> + 31, 32, 33, 34,
> + 41, 42, 43, 44,
> + 51, 52, 53, 54,
> + 61, 62, 63, 64,
> + 71, 72, 73, 74,
> + 81, 82, 83, 84,
> + };
> +
> + return array_loc[1][2];
> +}
> +
> +[test]
> +draw quad
> +probe all rgba (71, 72, 73, 74)
> +
> diff --git a/tests/hlsl-array-initializer-static.shader_test b/tests/hlsl-array-initializer-static.shader_test
> new file mode 100644
> index 00000000..f103dd9a
> --- /dev/null
> +++ b/tests/hlsl-array-initializer-static.shader_test
> @@ -0,0 +1,21 @@
> +[pixel shader]
> +static const float4 array_st[4][2] = {
> + 11, 12, 13, 14,
> + 21, 22, 23, 24,
> + 31, 32, 33, 34,
> + 41, 42, 43, 44,
> + 51, 52, 53, 54,
> + 61, 62, 63, 64,
> + 71, 72, 73, 74,
> + 81, 82, 83, 84,
> +};
> +
> +float4 main() : SV_TARGET
> +{
> + return array_st[2][1];
> +}
> +
> +[test]
> +draw quad
> +probe all rgba (61, 62, 63, 64)
> +
More information about the wine-devel
mailing list