[PATCH vkd3d 09/12] vkd3d-shader/hlsl: Flatten initializers.

[Francisco Casas]

Hello,

April 20, 2022 3:34 PM, "Zebediah Figura" <zfigura at codeweavers.com> wrote:

> 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.
> 

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.