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

[Zebediah Figura]

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?

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

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

> +            }
>               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).

It also aborts compilation somewhat unnecessarily.

(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[];
     };

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

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