Hello,
April 20, 2022 3:34 PM, "Zebediah Figura" zfigura@codeweavers.com wrote:
On 4/18/22 01:34, Giovanni Mascellani wrote:
From: Francisco Casas fcasas@codeweavers.com
I did some minor changes, so I'm removing Francisco's Signed-off-by.
libs/vkd3d-shader/hlsl.h | 6 ++ libs/vkd3d-shader/hlsl.y | 115 +++++++++++++++++++-- tests/hlsl-initializer-flatten.shader_test | 8 +- 3 files changed, 118 insertions(+), 11 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 802adf87..18b70853 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -726,6 +726,9 @@ struct hlsl_ir_function_decl *hlsl_get_func_decl(struct hlsl_ctx *ctx, const cha struct hlsl_type *hlsl_get_type(struct hlsl_scope *scope, const char *name, bool recursive); struct hlsl_ir_var *hlsl_get_var(struct hlsl_scope *scope, const char *name);
+struct hlsl_ir_node *hlsl_load_index(struct hlsl_ctx *ctx, struct list *instrs, struct
hlsl_ir_node *instr,
- unsigned int idx, const struct vkd3d_shader_location *loc);
struct hlsl_type *hlsl_new_array_type(struct hlsl_ctx *ctx, struct hlsl_type *basic_type, unsigned int array_size); struct hlsl_ir_node *hlsl_new_binary_expr(struct hlsl_ctx *ctx, enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg1, struct hlsl_ir_node *arg2); @@ -768,6 +771,9 @@ struct hlsl_ir_var *hlsl_new_var(struct hlsl_ctx *ctx, const char *name, struct struct hlsl_ir_load *hlsl_new_var_load(struct hlsl_ctx *ctx, struct hlsl_ir_var *var, const struct vkd3d_shader_location loc);
+struct hlsl_ir_node *hlsl_store_index(struct hlsl_ctx *ctx, struct list *instrs, struct
hlsl_ir_var *var,
- struct hlsl_ir_node *rhs, unsigned int idx, const struct vkd3d_shader_location *loc);
void hlsl_error(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc, enum vkd3d_shader_error error, const char *fmt, ...) VKD3D_PRINTF_FUNC(4, 5); void hlsl_fixme(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc,
Why are these no longer static? They're still only used in hlsl.y.
This is probably an artifact from reordering the patches. Some of my future patches use it outside hlsl.y, so I see no point in changing it now in a separate commit.
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 1ee1db4c..5b57a7bd 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1534,6 +1534,108 @@ static bool add_increment(struct hlsl_ctx *ctx, struct list *instrs, bool decrem return true; }
+static bool append_node_components(struct hlsl_ctx *ctx, struct list *instrs,
- struct hlsl_ir_node *node, struct hlsl_ir_node **comps, unsigned int *comps_count,
- const struct vkd3d_shader_location *loc)
+{
- struct hlsl_type *type = node->data_type;
- switch (type->type)
- {
- case HLSL_CLASS_SCALAR:
- case HLSL_CLASS_VECTOR:
- case HLSL_CLASS_MATRIX:
- {
- unsigned int idx;
- for (idx = 0; idx < type->dimy * type->dimx; idx++)
- {
- struct hlsl_ir_node *value;
- if (!(value = hlsl_load_index(ctx, instrs, node, idx, loc)))
- return false;
- comps[*comps_count] = value;
Nitpick, but you don't need a local variable for this.
Okay.
- *comps_count += 1;
- }
- break;
- }
- case HLSL_CLASS_STRUCT:
- {
- struct hlsl_struct_field *field;
- struct hlsl_ir_node *load;
- LIST_FOR_EACH_ENTRY(field, type->e.elements, struct hlsl_struct_field, entry)
- {
- if (!add_record_load(ctx, instrs, node, field, *loc))
- return false;
I'm a bit torn on this one. My knee-jerk reaction is that accessing reg_offset and field->type directly would be simpler [along the lines of prepend_input_struct_copy(), although that function has other considerations], but on reflection I'm less sure. It'd definitely avoid generating extra work for the parser, though...
To use hlsl_new_load() instead, we would have to: - Add an argument for the current reg_offset. - Add an argument for the original node (that contains the "node" in the current call). - For consistency, we would also have to change hlsl_load_index() from the previous case and add_array_load() in the next case.
So we would end up with a signature more like initialize_generic_var().
I can do that if it seems better.
- load = node_from_list(instrs);
- if (!append_node_components(ctx, instrs, load, comps, comps_count, loc))
- return false;
- }
- break;
- }
- case HLSL_CLASS_ARRAY:
- {
- struct hlsl_ir_constant *c;
- struct hlsl_ir_node *load;
- unsigned int i;
- for (i = 0; i < type->e.array.elements_count; i++)
- {
- if (!(c = hlsl_new_uint_constant(ctx, i, loc)))
- return false;
- list_add_tail(instrs, &c->node.entry);
- if (!add_array_load(ctx, instrs, node, &c->node, *loc))
- return false;
- load = node_from_list(instrs);
- if (!append_node_components(ctx, instrs, load, comps, comps_count, loc))
- return false;
- }
- break;
- }
- case HLSL_CLASS_OBJECT:
- {
- hlsl_fixme(ctx, loc, "Flattening of objects.");
- return false;
- }
Could this just fall into the scalar case?
Not for now, hlsl_load_index doesn't support objects. We still are missing tests for complex initializer with objects elements, and see what the native compiler does.
I think we should add the tests in a separate patch before supporting them.
- }
- return true;
+}
+static bool flatten_parse_initializer(struct hlsl_ctx *ctx, struct parse_initializer *initializer) +{
- unsigned int size = initializer_size(initializer);
- unsigned int new_args_count = 0;
- struct hlsl_ir_node **new_args;
- bool success = true;
- unsigned int i = 0;
- new_args = hlsl_alloc(ctx, size * sizeof(struct hlsl_ir_node *));
- if (!new_args)
- return false;
- for (i = 0; i < initializer->args_count; i++)
- {
- success = append_node_components(ctx, initializer->instrs, initializer->args[i], new_args,
- &new_args_count, &initializer->args[i]->loc);
- if (!success)
- break;
- }
- vkd3d_free(initializer->args);
- initializer->args = new_args;
- initializer->args_count = new_args_count;
- return success;
+}
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) @@ -1768,14 +1870,13 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t } else {
- if (v->initializer.args_count != size)
- if (!flatten_parse_initializer(ctx, &v->initializer))
{
- hlsl_fixme(ctx, &v->loc, "Flatten initializer.");
free_parse_initializer(&v->initializer); vkd3d_free(v); continue; }
- assert(v->initializer.args_count == size);
initialize_numeric_var(ctx, var, &v->initializer, 0, type, &initializer_offset); } }
This works, but I'm not sure that it's my favourite organization. Without having tried it, what seems maybe more idiomatically structured is to bring the hlsl_new_store() call into this loop, not unlike the way it's done for constructors. You could still keep the recursive quality of the function for the benefit of structs and arrays, but effectively pass the store index as an in-out parameter instead of comps_count. I think this would also simplify error handling.
In more concrete terms, I'm envisioning something like:
static void initialize_var_components(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_var *var, [in, out] unsigned int *store_index, struct hlsl_ir_node *instr)
which would be recursive, and called from what is currently initialize_numeric_var().
If I understand correctly, you are proposing to merge the "flatten" with the "initialize" functions. I tried it but I realized that it implies doing two types of recursion simultaneously, one on the "var" components and other on the "initializer->args"·s components.
Which is possible if we either: - Explicitly keep the call stack of one of the recursions. I assume the argument index and the call stack of "initializer->args". - Having a way of quickly map the index of a component (I assume that's what you meant with "store_index") to a register offset for any data type, or do a whole recursion each time we need that value.
I concluded that it is much more readable to keep the "flatten" separated from the "initialize" even if we mix those into one single function.
I still applied the formatting changes that Mystral suggested.