[PATCH vkd3d] vkd3d-shader/hlsl: Support array initializers.
Francisco Casas
fcasas at codeweavers.com
Mon Jan 3 11:00:27 CST 2022
December 30, 2021 2:55 PM, "Zebediah Figura (she/her)" <zfigura at codeweavers.com> wrote:
> On 12/29/21 08:51, Francisco Casas wrote:
>
>> 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.
>
> 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.
>
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.
> 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.
Will do, thanks for the tip.
>
>> Signed-off-by: Francisco Casas <fcasas at 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.
>
> 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.
Okay, I will keep the original function and just put the right calls to release memory on each
place.
More information about the wine-devel
mailing list