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

[Francisco Casas]

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.

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.

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