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

[Giovanni Mascellani]

Hi,

I think the patch generally looks good, but there are a few details to fix.

Il 28/04/22 15:32, Giovanni Mascellani ha scritto:
> @@ -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");
> +

I don't understand what this should be useful for, is it perhaps a 
leftover of development?

>       return type;
>   }
>   
> @@ -410,6 +415,7 @@ unsigned int hlsl_type_component_count(struct hlsl_type *type)
>       }
>       if (type->type == HLSL_CLASS_ARRAY)
>       {
> +        assert(type->e.array.elements_count != HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT);
>           return hlsl_type_component_count(type->e.array.type) * type->e.array.elements_count;
>       }
>       if (type->type != HLSL_CLASS_STRUCT)
> @@ -1023,7 +1029,12 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const stru
>               }
>   
>               for (t = type; t->type == HLSL_CLASS_ARRAY; t = t->e.array.type)
> -                vkd3d_string_buffer_printf(string, "[%u]", t->e.array.elements_count);
> +            {
> +                if (t->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
> +                    vkd3d_string_buffer_printf(string, "[<undefined>]");
> +                else
> +                    vkd3d_string_buffer_printf(string, "[%u]", t->e.array.elements_count);
> +            }

I guess elements_count should never be IMPLICIT here, should it? As soon 
as the hunk in declare_vars() is done, all the counts are known.

At any rate, if it can happen that the IMPLICIT branch can be taken 
here, then I'd just use "[]" instead of "[<undefined>]", which is the 
standard C way.

>               return string;
>           }
>   
> diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
> index 28b2ff1b..94597999 100644
> --- a/libs/vkd3d-shader/hlsl.h
> +++ b/libs/vkd3d-shader/hlsl.h
> @@ -227,6 +227,8 @@ struct hlsl_src
>   
>   #define HLSL_MODIFIERS_MAJORITY_MASK (HLSL_MODIFIER_ROW_MAJOR | HLSL_MODIFIER_COLUMN_MAJOR)
>   
> +#define HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT 0xffffffff
> +
>   struct hlsl_reg_reservation
>   {
>       char type;
> 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);

Maybe it's just me, but here I would also assert that i == 
v->arrays.count - 1. This should already be forced by the parser, but 
it's a useful code documentation anyway.

> +                v->arrays.sizes[i] = (size + elem_components - 1)/elem_components;

Why "+ elem_components - 1", given that immediately below we error out 
if size is not a multiple of elem_components?

Also, notice that elem_components could be zero. You should handle that 
gracefully.

> +
> +                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;
> +                }
> +            }
>               type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]);
> +        }
>           vkd3d_free(v->arrays.sizes);
>   

Thanks, Giovanni.