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@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@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]); } - 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) +
On 12/29/21 08:51, Francisco Casas wrote:
I don't think recursion should be necessary at all.
Rather, since the "offset" in hlsl_deref is designed not to care about the underlying division of the HLSL struct or array type, we should be able to do something as simple as iterating over initializer elements and sequentially storing them to the variable. In that case we don't even need to care about the type class, and can even use this code for numeric types.
Some more things I'd like to see:
* Tests for initializers that are too large or too small.
* Tests for struct initializers, to make sure that they aren't being broken by this patch. I think we have numeric type initializer tests already but if not we should add some of those too.
Splitting out the tests into separate patches (and probably ordering them before the fix) is generally appreciated.
Yes, I'm sorry, but it's very ugly. It also begs the question of why you need to change this call at all; implementing initializers should only be a matter of adding new instructions, not changing the way variables are defined.
December 30, 2021 2:55 PM, "Zebediah Figura (she/her)" zfigura@codeweavers.com wrote:
I understand that hlsl_deref doesn't care about the division of the struct, however, I still think recursion is necessary.
So far, an hlsl_type is a recursive data type: It could be an HLSL_CLASS_STRUCT that contains a HLSL_CLASS_ARRAY field, that contains HLSL_CLASS_STRUCT elements, and so on (not necessarily switching between arrays and structs). As in:
struct aaa { struct { int2 ccc; float4 ddd; } bbbs[3]; };
float4 PSMain() : SV_TARGET { struct aaa foo = {11,12,13,14,15,16,21,22,23,24,25,26,31,32,33,34,35,36}; return foo.bbbs[1].ddd; // 23.0, 24.0, 25.0, 26.0 }
So, to iterate all its leaves I would eventually need to recurse (or alternatively, to work with a "stack" of type pointers which is similar). And it is necessary to do that to know the base_type of each field/element to add the right implicit cast.
I think I can subjectively improve the quality of the code, e.g. eliminating the need for initializer_i, but not abandoning recursion unless we only want to implement arrays of basic types or of completely "flat" structs.
Will do, thanks for the tip.
Okay, I will keep the original function and just put the right calls to release memory on each place.
On 1/3/22 11:00, Francisco Casas wrote:
Hrm, I see the problem; I was forgetting about padding between struct members (and array elements). I guess recursion is the best solution in that case, then.
I would like to see this patch be more split up, though. At the moment it doesn't just support array initializers, it also supports nested structs, and flat initializers for non-scalars. All of that should be split into separate patches.
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:
Why can't this be handled by initialize_var_recursive() as the general case?
-
Francisco Casas -
Giovanni Mascellani -
Zebediah Figura (she/her)