[PATCH vkd3d 2/8] vkd3d-shader/hlsl: Support initialization of implicit size arrays.

[Francisco Casas]

Hello,

May 6, 2022 4:38 AM, "Giovanni Mascellani" <gmascellani at codeweavers.com> wrote:

> Hi,
> 
> thanks for reworking again your patches, unfortunately it seems to me that there are still points
> that can be enhanced.
> 

No problem, thanks for having me patience.

> Il 05/05/22 15:16, Giovanni Mascellani ha scritto:
> 
>> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
>> index 44e4964f..9fe454d0 100644
>> --- a/libs/vkd3d-shader/hlsl.y
>> +++ b/libs/vkd3d-shader/hlsl.y
>> @@ -779,6 +779,7 @@ static struct list *gen_struct_fields(struct hlsl_ctx *ctx,
>> return NULL;
>> LIST_FOR_EACH_ENTRY_SAFE(v, v_next, fields, struct parse_variable_def, entry)
>> {
>> + bool unbounded_res_array = false;
>> unsigned int i;
>>> if (!(field = hlsl_alloc(ctx, sizeof(*field))))
>> @@ -789,7 +790,33 @@ static struct list *gen_struct_fields(struct hlsl_ctx *ctx,
>>> field->type = type;
>> for (i = 0; i < v->arrays.count; ++i)
>> + {
>> + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>> + {
>> + if (i < v->arrays.count - 1)
>> + {
>> + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
>> + "Inner array size cannot be implicit.");
>> + }
>> + else if (type->type == HLSL_CLASS_OBJECT)
>> + {
>> + unbounded_res_array = true;
>> + }
>> + else
>> + {
>> + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
>> + "Implicit size arrays not allowed in struct fields.");
>> + }
>> + }
>> field->type = hlsl_new_array_type(ctx, field->type, v->arrays.sizes[i]);
>> + }
>> + if (unbounded_res_array)
>> + {
>> + hlsl_fixme(ctx, &v->loc, "Unbounded resource arrays as struct fields.");
>> + free_parse_variable_def(v);
>> + vkd3d_free(field);
>> + continue;
>> + }
>> vkd3d_free(v->arrays.sizes);
>> field->loc = v->loc;
>> field->name = v->name;
> 
> Here we are in the context of a struct field, where is seems that any kind of unbounded array is
> forbidden, isn't it? I mean, it should be an hlsl_error(), not an hlsl_fixme(): this program fails
> to compile with native:
> ---
> struct ss
> {
> Texture2D x[];
> };
> 
> float4 main() : sv_target
> {
> struct ss arr;
> 
> return 0.0;
> }
> ---
> 

As Zebediah pointed out in her reviews of my previous patches, in ps_5_1 and vs_5_1, arrays of 
textures are processed as a different datatype, and they are allowed to be implicit, and even be
struct members. They are called "unbounded resource arrays".

For instance, your example actually compiles with ps_5_1.

So it was decided to drop those variables for now.


> Once this is taken into account, it seems that any kind of implicit array is just forbidden inside
> a struct definition, so you can avoid different cases depending on the underlying type: if you see
> _COUNT_IMPLICIT inside gen_struct_field() you just hlsl_error() out (just like you do for typedefs,
> for instance). This would also agree with the native d3dcompiler's error message: "array dimensions
> of struct/class members must be explicit", though that's not necessarily a strong argument. Or an
> argument at all. Just a funny coincidence.
> 

Well, as I mentioned that's not totally true in ps_5_1.

I could check that the profile is >= 5.1 in the conditionals.


>> @@ -1601,11 +1634,67 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type
>> *basic_t
>>> LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry)
>> {
>> + bool unbounded_res_array = false;
>> unsigned int i;
>>> type = basic_type;
>> +
>> for (i = 0; i < v->arrays.count; ++i)
>> + {
>> + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>> + {
>> + unsigned int size = initializer_size(&v->initializer);
>> + unsigned int elem_components = hlsl_type_component_count(type);
>> +
>> + if (i < v->arrays.count - 1)
>> + {
>> + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
>> + "Inner array size cannot be implicit.");
>> + free_parse_initializer(&v->initializer);
>> + v->initializer.args_count = 0;
>> + }
>> + else if (elem_components == 0)
>> + {
>> + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
>> + "Cannot declare an implicit size array of a size 0 type.");
>> + free_parse_initializer(&v->initializer);
>> + v->initializer.args_count = 0;
>> + }
>> + else if (size == 0)
>> + {
>> + if (type->type == HLSL_CLASS_OBJECT)
>> + {
>> + unbounded_res_array = 1;
>> + }
>> + else
>> + {
>> + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
>> + "Implicit size arrays need to be initialized.");
>> + free_parse_initializer(&v->initializer);
>> + v->initializer.args_count = 0;
>> + }
>> + }
>> + else if (size % elem_components != 0)
>> + {
>> + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
>> + "Cannot initialize implicit size array with %u components, expected a multiple of %u.",
>> + size, elem_components);
>> + free_parse_initializer(&v->initializer);
>> + v->initializer.args_count = 0;
>> + }
>> + else
>> + {
>> + v->arrays.sizes[i] = size / elem_components;
>> + }
>> + }
>> type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]);
>> + }
>> + if (unbounded_res_array)
>> + {
>> + hlsl_fixme(ctx, &v->loc, "Unbounded resource arrays.");
>> + free_parse_variable_def(v);
>> + continue;
>> + }
>> vkd3d_free(v->arrays.sizes);
>>> if (type->type != HLSL_CLASS_MATRIX)
> 
> Isn't the case "type->type == HLSL_CLASS_OBJECT" already covered by "elem_components == 0"? When
> type is of OBJECT class, then hlsl_type_component_count() always spits out zero (and logs an ERR();
> arguably it should return an error condition which should be handled by the caller, but maybe this
> issue can be postponed).
> 

I see. I have been pushing for hlsl_type_component_count() to return 1 with objects, so that 
initializers work correctly on structs that have object members, like in:

---
Texture2D tex;

struct foo
{
    float2 aa;
    Texture2D bb[2];
    float4 cc;
};

float4 main() : sv_target
{
    struct foo q = {10, 20, tex, tex, 30, 40, 50, 60};

    return q.cc;
}
---

This indeed seems necessary for the condition '(size == 0)' to not be redundant.

Instead of removing it, I suggest considering first a patch so that
hlsl_type_component_count() returns 1 for objects. I will get to work on it.


> BTW, notice that you can use "true" instead of "1" to assign to a boolean variable.
> 

Agh! Sorry! It is just a habit.


> Giovanni.