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

[Francisco Casas]

Hello,

April 28, 2022 10:49 AM, "Giovanni Mascellani" <gmascellani at codeweavers.com> wrote:

> Hi,
> 
> I think the patch generally looks good, but there are a few details to fix.
> 
> Il 28/04/22 15:32, Giovanni Mascellani ha scritto:
> 
>> @@ -328,6 +330,9 @@ struct hlsl_type *hlsl_new_array_type(struct hlsl_ctx *ctx, struct hlsl_type
>> *ba
>>> list_add_tail(&ctx->types, &type->entry);
>>> + if (type->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>> + TRACE("Implicit element count.\n");
>> +
> 
> I don't understand what this should be useful for, is it perhaps a leftover of development?
> 

Yes, probably a leftover. I think I was worried adding the special case of implicit arrays could
generate bugs in other parts of the code if they didn't consider it.
But that shouldn't be the case with this implementation since this special case is not present
after the initialization.


>> return type;
>> }
>>> @@ -410,6 +415,7 @@ unsigned int hlsl_type_component_count(struct hlsl_type *type)
>> }
>> if (type->type == HLSL_CLASS_ARRAY)
>> {
>> + assert(type->e.array.elements_count != HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT);
>> return hlsl_type_component_count(type->e.array.type) * type->e.array.elements_count;
>> }
>> if (type->type != HLSL_CLASS_STRUCT)
>> @@ -1023,7 +1029,12 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const
>> stru
>> }
>>> for (t = type; t->type == HLSL_CLASS_ARRAY; t = t->e.array.type)
>> - vkd3d_string_buffer_printf(string, "[%u]", t->e.array.elements_count);
>> + {
>> + if (t->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>> + vkd3d_string_buffer_printf(string, "[<undefined>]");
>> + else
>> + vkd3d_string_buffer_printf(string, "[%u]", t->e.array.elements_count);
>> + }
> 
> I guess elements_count should never be IMPLICIT here, should it? As soon as the hunk in
> declare_vars() is done, all the counts are known.
> 

Yes, but I think we better cover all cases in a function we may be using for debugging purposes.

> At any rate, if it can happen that the IMPLICIT branch can be taken here, then I'd just use "[]"
> instead of "[<undefined>]", which is the standard C way.
> 

I think I opted for this verbose alternative since finding an implicit size array in the wild would 
be an oddity worth emphasizing. But maybe I was paranoid. I will change it to "[]". 

>> return string;
>> }
>>> diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
>> index 28b2ff1b..94597999 100644
>> --- a/libs/vkd3d-shader/hlsl.h
>> +++ b/libs/vkd3d-shader/hlsl.h
>> @@ -227,6 +227,8 @@ struct hlsl_src
>>> #define HLSL_MODIFIERS_MAJORITY_MASK (HLSL_MODIFIER_ROW_MAJOR | HLSL_MODIFIER_COLUMN_MAJOR)
>>> +#define HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT 0xffffffff
>> +
>> struct hlsl_reg_reservation
>> {
>> char type;
>> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
>> index 905dbfc5..e7fe74d8 100644
>> --- a/libs/vkd3d-shader/hlsl.y
>> +++ b/libs/vkd3d-shader/hlsl.y
>> @@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type
>> *basic_t
>>> 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);
>> +
>> + assert(v->initializer.args_count);
> 
> Maybe it's just me, but here I would also assert that i == v->arrays.count - 1. This should already
> be forced by the parser, but it's a useful code documentation anyway.
> 

Sounds good.

>> + v->arrays.sizes[i] = (size + elem_components - 1)/elem_components;
> 
> Why "+ elem_components - 1", given that immediately below we error out if size is not a multiple of
> elem_components?
> 
> Also, notice that elem_components could be zero. You should handle that gracefully.
> 

These are good points, I will update the patch.

>> +
>> + if (size % elem_components != 0)
>> + {
>> + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
>> + "Cannot initialize implicit array with %u components, expected a multiple of %u.",
>> + size, elem_components);
>> + free_parse_initializer(&v->initializer);
>> + v->initializer.args_count = 0;
>> + }
>> + }
>> type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]);
>> + }
>> vkd3d_free(v->arrays.sizes);
> 
> Thanks, Giovanni.