[PATCH vkd3d 09/12] vkd3d-shader/hlsl: Flatten initializers.
Zebediah Figura
zfigura at codeweavers.com
Wed Apr 20 14:33:44 CDT 2022
On 4/18/22 01:34, Giovanni Mascellani wrote:
> From: Francisco Casas <fcasas at 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.
> 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.
> + *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...
> +
> + 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?
> + }
> +
> + 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().
More information about the wine-devel
mailing list