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

[Zebediah Figura]

On 4/28/22 16:30, Francisco Casas wrote:
> April 28, 2022 4:22 PM, "Zebediah Figura" <zfigura at codeweavers.com> wrote:
> 
>> 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 :-)
>>
> 
> Well, the structure of the parsing rule ensures that only the most external array could have
> IMPLICIT side. This is related to the
> assert(i == v->arrays.count - 1);
> suggested by Giovanni.
> 
>> I don't see any handling for braceless initializers in this patch, though; am I missing something?
>>
> 
> The error is added to the "variable_def:" rule.
> But then again, maybe it is not so clear that the parse rule ensures that only the most external array
> can have IMPLICIT size. I could add the same assertion here too.

Right, the assert works to clarify.

I'm not sure whether "float a[2][]" should be a syntax error, though. As 
elsewhere, it'd be more helpful to explain why it's wrong, and probably 
also to avoid failing compilation immediately.

> By the way, I am not sure if you changed your stance on checking the implicit size arrays at the parse
> level. I would prefer to keep it there... if I find a way of also handling these unbounded resource
> arrays nicely.

Well, it at least can't be done until we have type information, which 
means it'd need to be deferred until declare_vars and 
gen_struct_fields(). That sounds like conceptually the right place for 
it, to me.

> 
>>> 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.");
> 
> Here ^
> 
>>> + 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.)
> 
> Hmm, I will see where I can add a FIXME for those cases for now then.

It's not something that this patch necessarily needs to handle, even 
with a FIXME, but it should inform how the code is structured, which is 
why I bring it up.