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

[Francisco Casas]

Hello,

April 28, 2022 2:22 PM, "Zebediah Figura" <zfigura at codeweavers.com> wrote:

> On 4/28/22 08:32, Giovanni Mascellani wrote:
> 
>> @@ -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");
>> +
>> return type;
>> }
> 
> When can this currently happen?
> 

Huh, actually this shouldn't happen in the current patch as it is at least. I will remove it.

>> 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);
>> +
>> + v->arrays.sizes[i] = (size + elem_components - 1)/elem_components;
>> +
>> + 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;
>> + }
> 
> This doesn't seem like it'll do the right thing for implicit sizes on an inner array, especially
> considering that the right thing is probably to fail compilation.
> 
> It also won't do the right thing for initializers without braces (viz. also fail compilation).
> 

These cases are checked in the parse rules introduced in this patch.

> I think we need tests for all of these corner cases, and also a test for missing initializers.
> 

Okay, adding them.

>> + }
>> type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]);
>> + }
>> vkd3d_free(v->arrays.sizes);
>>> if (type->type != HLSL_CLASS_MATRIX)
>> @@ -2464,6 +2484,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct
>> hl
>> %token <name> TYPE_IDENTIFIER
>>> %type <arrays> arrays
>> +%type <arrays> implicit_arrays
>>> %type <assign_op> assign_op
>>> @@ -3108,7 +3129,7 @@ variables_def:
>> }
>>> variable_decl:
>> - any_identifier arrays colon_attribute
>> + any_identifier implicit_arrays colon_attribute
>> {
>> $$ = hlsl_alloc(ctx, sizeof(*$$));
>> $$->loc = @1;
>> @@ -3137,6 +3158,15 @@ state_block:
>>> variable_def:
>> variable_decl
>> + {
>> + if ($$->arrays.sizes && $$->arrays.sizes[$$->arrays.count - 1] ==
>> HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>> + {
>> + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER,
>> + "Implicit array requires initializer.");
>> + free_parse_variable_def($$);
>> + YYABORT;
>> + }
>> + }
> 
> This won't work for unbounded resource arrays, which can be declared with an empty pair of brackets
> (and no initializer).
> 

I see, I didn't knew about those.

> It also aborts compilation somewhat unnecessarily.
> 

AFAIK (except for unbounded resource arrays) the native compiler doesn't allow implicit size arrays
without initializer, so aborting seemed logical.

> (As a side note, perhaps we should use zero instead of UINT_MAX. Unbounded resources in shader
> model 5.1 are encoded as zero in the reflection data, whereas declaring a resource array as e.g.
> "Texture2D t[0xffffffff]" yields 0xffffffff instead, although the shader bytecode is identical.)
> 
> As a special extra, this code is apparently valid, and I think deserves to be a test case:
> 
> struct apple
> {
> Texture2D t[];
> };
> 

Hmm, this is interesting. Maybe it is intended for input semantics?
I will investigate.

> uniform struct apple a;
> 
> float4 main() : sv_target
> {
> return a.t[0].Load(0);
> }
> 
> [We don't currently have support for unbounded arrays in the shader_runner infrastructure, though,
> so it doesn't urgently need to be a test case. Alternatively it could be written in C.]
> 

Okay, I will see if I can write a C test, if not now, later.

>> | variable_decl '=' complex_initializer
>> {
>> $$ = $1;
>> @@ -3188,6 +3218,24 @@ arrays:
>> $$.sizes[$$.count++] = size;
>> }
>>> +implicit_arrays:
>> + arrays
>> + | '[' ']' arrays
>> + {
>> + uint32_t *new_array;
>> +
>> + $$ = $3;
>> +
>> + if (!(new_array = hlsl_realloc(ctx, $$.sizes, ($$.count + 1) * sizeof(*new_array))))
>> + {
>> + vkd3d_free($$.sizes);
>> + YYABORT;
>> + }
>> +
>> + $$.sizes = new_array;
>> + $$.sizes[$$.count++] = HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT;
>> + }
>> +
> 
> I think this should be part of the "arrays" rule, actually. Besides being simpler YACC, it allows
> us to generate more helpful messages than "syntax error" if someone tries to use an empty array
> size in an invalid context.

Okay, I will try moving everything outside the parser rules. But we will need to introduce several
checks to ensure that all the array sizes are defined where they are needed, including typedefs,
casts, struct members. 

Now that I think about it, it may also be worth testing if implicit size arrays are allowed in function
argument and return types.