[PATCH vkd3d 3/6] vkd3d-shader/hlsl: Support complex numeric initializers.
Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> --- Makefile.am | 1 - libs/vkd3d-shader/hlsl.y | 93 +++++++++++++++++++++++++++++++--------- 2 files changed, 73 insertions(+), 21 deletions(-) diff --git a/Makefile.am b/Makefile.am index 491537f6..85611daf 100644 --- a/Makefile.am +++ b/Makefile.am @@ -310,7 +310,6 @@ XFAIL_TESTS = \ tests/hlsl-initializer-invalid-arg-count.shader_test \ tests/hlsl-initializer-local-array.shader_test \ tests/hlsl-initializer-nested.shader_test \ - tests/hlsl-initializer-numeric.shader_test \ tests/hlsl-initializer-static-array.shader_test \ tests/hlsl-initializer-struct.shader_test \ tests/hlsl-bool-cast.shader_test \ diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 4ec36b76..bcc9e4d7 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1321,6 +1321,51 @@ static bool add_increment(struct hlsl_ctx *ctx, struct list *instrs, bool decrem return true; } +static void initialize_numeric_var(struct hlsl_ctx *ctx, struct hlsl_ir_var *var, + struct parse_initializer *initializer, unsigned int reg_offset, struct hlsl_type *type, + unsigned int *initializer_offset) +{ + unsigned int i; + + if (type->type == HLSL_CLASS_MATRIX) + hlsl_fixme(ctx, &var->loc, "Matrix initializer."); + + for (i = 0; i < type->dimx; i++) + { + struct hlsl_ir_store *store; + struct hlsl_ir_constant *c; + struct hlsl_ir_node *node; + + node = initializer->args[*initializer_offset]; + *initializer_offset += 1; + + if (!(node = add_implicit_conversion(ctx, initializer->instrs, node, + hlsl_get_scalar_type(ctx, type->base_type), &node->loc))) + return; + + if (hlsl_type_is_single_reg(var->data_type)) + { + if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc))) + return; + list_add_tail(initializer->instrs, &c->node.entry); + + if (!(store = hlsl_new_store(ctx, var, &c->node, node, (1u << i), node->loc))) + return; + } + else + { + if (!(c = hlsl_new_uint_constant(ctx, reg_offset + i, node->loc))) + return; + list_add_tail(initializer->instrs, &c->node.entry); + + if (!(store = hlsl_new_store(ctx, var, &c->node, node, 0, node->loc))) + return; + } + + list_add_tail(initializer->instrs, &store->node.entry); + } +} + static void struct_var_initializer(struct hlsl_ctx *ctx, struct list *list, struct hlsl_ir_var *var, struct parse_initializer *initializer) { @@ -1492,20 +1537,16 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t 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 (type->type <= HLSL_CLASS_LAST_NUMERIC && v->initializer.args_count != 1 + && type->dimx * type->dimy != size) { - 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; - } + 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, v->initializer.args_count); + 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) @@ -1540,23 +1581,35 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t vkd3d_free(v); continue; } + if (v->initializer.args_count > 1) { - hlsl_fixme(ctx, &v->loc, "Complex initializer."); - free_parse_initializer(&v->initializer); - vkd3d_free(v); - continue; + unsigned int initializer_offset = 0; + + if (v->initializer.args_count != size) + { + hlsl_fixme(ctx, &v->loc, "Flatten initializer."); + free_parse_initializer(&v->initializer); + vkd3d_free(v); + continue; + } + + initialize_numeric_var(ctx, var, &v->initializer, 0, type, &initializer_offset); } + else + { + struct hlsl_ir_load *load = hlsl_new_var_load(ctx, var, var->loc); - 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); + 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 (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.args); vkd3d_free(v->initializer.instrs); } -- 2.25.1
Hi, the patch looks mostly ok, with a few comments. Il 15/02/22 21:17, Francisco Casas ha scritto:
+static void initialize_numeric_var(struct hlsl_ctx *ctx, struct hlsl_ir_var *var, + struct parse_initializer *initializer, unsigned int reg_offset, struct hlsl_type *type, + unsigned int *initializer_offset) +{
Isn't "type" here the same thing as "var->data_type" in the only call to this function? If so, you could avoid passing "type" explicitly.
+ unsigned int i; + + if (type->type == HLSL_CLASS_MATRIX) + hlsl_fixme(ctx, &var->loc, "Matrix initializer."); + + for (i = 0; i < type->dimx; i++) + { + struct hlsl_ir_store *store; + struct hlsl_ir_constant *c; + struct hlsl_ir_node *node; + + node = initializer->args[*initializer_offset]; + *initializer_offset += 1; + + if (!(node = add_implicit_conversion(ctx, initializer->instrs, node, + hlsl_get_scalar_type(ctx, type->base_type), &node->loc))) + return; + + if (hlsl_type_is_single_reg(var->data_type)) + { + if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc))) + return; + list_add_tail(initializer->instrs, &c->node.entry); + + if (!(store = hlsl_new_store(ctx, var, &c->node, node, (1u << i), node->loc))) + return; + } + else + { + if (!(c = hlsl_new_uint_constant(ctx, reg_offset + i, node->loc))) + return; + list_add_tail(initializer->instrs, &c->node.entry); + + if (!(store = hlsl_new_store(ctx, var, &c->node, node, 0, node->loc))) + return; + }
Again under the assumption that "type == var->data_type", here it seems that you're handling the matrix case, while it is marked as fixme above. I guess the reason for the fixme is that you're only iterating on dimx? Still, I find it a bit confusing. If you consider the matrix case not (yet) supported, I'd just avoid to add pieces of it until you're really supporting it. Though maybe I missing something. Thanks, Giovanni.
Hello February 16, 2022 2:14 PM, "Giovanni Mascellani" <gmascellani(a)codeweavers.com> wrote:
Hi,
the patch looks mostly ok, with a few comments.
Il 15/02/22 21:17, Francisco Casas ha scritto:
+static void initialize_numeric_var(struct hlsl_ctx *ctx, struct hlsl_ir_var *var, + struct parse_initializer *initializer, unsigned int reg_offset, struct hlsl_type *type, + unsigned int *initializer_offset) +{
Isn't "type" here the same thing as "var->data_type" in the only call to this function? If so, you could avoid passing "type" explicitly.
For now it is the same thing but 4 patches ahead (in the continuation of this batch) that will no longer be true. The idea is that var may also be a struct or an array, and type would be the type of the numeric field inside that we want to initialize.
+ unsigned int i; + + if (type->type == HLSL_CLASS_MATRIX) + hlsl_fixme(ctx, &var->loc, "Matrix initializer."); + + for (i = 0; i < type->dimx; i++) + { + struct hlsl_ir_store *store; + struct hlsl_ir_constant *c; + struct hlsl_ir_node *node; + + node = initializer->args[*initializer_offset]; + *initializer_offset += 1; + + if (!(node = add_implicit_conversion(ctx, initializer->instrs, node, + hlsl_get_scalar_type(ctx, type->base_type), &node->loc))) + return; + + if (hlsl_type_is_single_reg(var->data_type)) + { + if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc))) + return; + list_add_tail(initializer->instrs, &c->node.entry); + + if (!(store = hlsl_new_store(ctx, var, &c->node, node, (1u << i), node->loc))) + return; + } + else + { + if (!(c = hlsl_new_uint_constant(ctx, reg_offset + i, node->loc))) + return; + list_add_tail(initializer->instrs, &c->node.entry); + + if (!(store = hlsl_new_store(ctx, var, &c->node, node, 0, node->loc))) + return; + }
Again under the assumption that "type == var->data_type", here it seems that you're handling the matrix case, while it is marked as fixme above. I guess the reason for the fixme is that you're only iterating on dimx? Still, I find it a bit confusing. If you consider the matrix case not (yet) supported, I'd just avoid to add pieces of it until you're really supporting it. Though maybe I missing something.
As before, "type == var->data_type" won't hold after 33e27ff7 vkd3d-shader/hlsl: Use generic variable initializer for struct fields. which would have been the 7th patch of this batch if I hadn't split it. So it is not just for the matrix case. (I implemented matrix initializers too but they are 10 initializer patches ahead.) Best regards, Francisco
Hi, Il 16/02/22 18:59, Francisco Casas ha scritto:
For now it is the same thing but 4 patches ahead (in the continuation of this batch) that will no longer be true. The idea is that var may also be a struct or an array, and type would be the type of the numeric field inside that we want to initialize.
Yeah, it makes sense. I think the patches in this series are correct, meaning that as far as I can see they do not introduce errors and the general structure of the code looks fine. Clearly they leave the code in a very "in the making" state, but that's reasonable considering that you still have other work to submit and are working actively on this thing, so I am signing off on them anyway. Once you are done with all your patches I might still have a few comments, but they are minor enough that to me it makes sense to first let the patches in and then, at the end, fix the small leftovers. Giovanni.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> Il 15/02/22 21:17, Francisco Casas ha scritto:
Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> --- Makefile.am | 1 - libs/vkd3d-shader/hlsl.y | 93 +++++++++++++++++++++++++++++++--------- 2 files changed, 73 insertions(+), 21 deletions(-)
diff --git a/Makefile.am b/Makefile.am index 491537f6..85611daf 100644 --- a/Makefile.am +++ b/Makefile.am @@ -310,7 +310,6 @@ XFAIL_TESTS = \ tests/hlsl-initializer-invalid-arg-count.shader_test \ tests/hlsl-initializer-local-array.shader_test \ tests/hlsl-initializer-nested.shader_test \ - tests/hlsl-initializer-numeric.shader_test \ tests/hlsl-initializer-static-array.shader_test \ tests/hlsl-initializer-struct.shader_test \ tests/hlsl-bool-cast.shader_test \ diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 4ec36b76..bcc9e4d7 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1321,6 +1321,51 @@ static bool add_increment(struct hlsl_ctx *ctx, struct list *instrs, bool decrem return true; }
+static void initialize_numeric_var(struct hlsl_ctx *ctx, struct hlsl_ir_var *var, + struct parse_initializer *initializer, unsigned int reg_offset, struct hlsl_type *type, + unsigned int *initializer_offset) +{ + unsigned int i; + + if (type->type == HLSL_CLASS_MATRIX) + hlsl_fixme(ctx, &var->loc, "Matrix initializer."); + + for (i = 0; i < type->dimx; i++) + { + struct hlsl_ir_store *store; + struct hlsl_ir_constant *c; + struct hlsl_ir_node *node; + + node = initializer->args[*initializer_offset]; + *initializer_offset += 1; + + if (!(node = add_implicit_conversion(ctx, initializer->instrs, node, + hlsl_get_scalar_type(ctx, type->base_type), &node->loc))) + return; + + if (hlsl_type_is_single_reg(var->data_type)) + { + if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc))) + return; + list_add_tail(initializer->instrs, &c->node.entry); + + if (!(store = hlsl_new_store(ctx, var, &c->node, node, (1u << i), node->loc))) + return; + } + else + { + if (!(c = hlsl_new_uint_constant(ctx, reg_offset + i, node->loc))) + return; + list_add_tail(initializer->instrs, &c->node.entry); + + if (!(store = hlsl_new_store(ctx, var, &c->node, node, 0, node->loc))) + return; + } + + list_add_tail(initializer->instrs, &store->node.entry); + } +} + static void struct_var_initializer(struct hlsl_ctx *ctx, struct list *list, struct hlsl_ir_var *var, struct parse_initializer *initializer) { @@ -1492,20 +1537,16 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t 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 (type->type <= HLSL_CLASS_LAST_NUMERIC && v->initializer.args_count != 1 + && type->dimx * type->dimy != size) { - 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; - } + 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, v->initializer.args_count); + 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) @@ -1540,23 +1581,35 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t vkd3d_free(v); continue; } + if (v->initializer.args_count > 1) { - hlsl_fixme(ctx, &v->loc, "Complex initializer."); - free_parse_initializer(&v->initializer); - vkd3d_free(v); - continue; + unsigned int initializer_offset = 0; + + if (v->initializer.args_count != size) + { + hlsl_fixme(ctx, &v->loc, "Flatten initializer."); + free_parse_initializer(&v->initializer); + vkd3d_free(v); + continue; + } + + initialize_numeric_var(ctx, var, &v->initializer, 0, type, &initializer_offset); } + else + { + struct hlsl_ir_load *load = hlsl_new_var_load(ctx, var, var->loc);
- 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); + 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 (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.args); vkd3d_free(v->initializer.instrs); }
participants (2)
-
Francisco Casas -
Giovanni Mascellani