From: "Francisco Casas" <fcasas@codeweavers.com>
Subject: Re: [PATCH vkd3d] vkd3d-shader/hlsl: Support array initializers.
Message-Id: <e9d68a7096fb0bd71900b3c12cb2b600@codeweavers.com>
Date: Mon, 03 Jan 2022 17:00:27 +0000
In-Reply-To: <4b188ac9-69d2-eaac-7984-881aec4ac55f@codeweavers.com>
References: <4b188ac9-69d2-eaac-7984-881aec4ac55f@codeweavers.com> <20211229145135.123336-1-fcasas@codeweavers.com>

December 30, 2021 2:55 PM, "Zebediah Figura (she/her)" <zfigura@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@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.