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

[Zebediah Figura]

On 4/28/22 14:45, Francisco Casas wrote:
>>> 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.

Oh, I see, I wasn't actually correctly reading the parser rule. That 
makes sense.

On the other hand, handling IMPLICIT inside of the loop, without any 
checks, was one of the things that made me think "we're not handling 
inner arrays correctly", so arguably something deserves changing here :-)

I don't see any handling for braceless initializers in this patch, 
though; am I missing something?

> 
>> 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.

Failing compilation is fine, but in general I think we want to avoid 
aborting if we can help it. We should only abort if we really can't 
continue parsing, e.g. if we encounter a syntax error. That way, if 
there are multiple errors, the (HLSL) programmer can deal with them all 
at once.

> 
>> (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.

Not for input semantics, but rather for bound resources. Shader model 
5.0 allows declaring arrays of textures, and shader model 5.1 (and 
Direct3D 12) allows declaring "unbounded" arrays, as e.g. "Texture2D 
t[]" or "Texture2D t[0]".

Normally textures aren't declared as part of a struct, but I was curious 
to see if it was legal, and it turns out it is. (Although the reflection 
type information that's generated isn't quite correct.)