Re: [PATCH vkd3d 3/6] vkd3d-shader/hlsl: Support complex numeric initializers.

[Giovanni Mascellani]

Hi,

the patch looks mostly ok, with a few comments.

Il 15/02/22 21:17, Francisco Casas ha scritto:
> +static void initialize_numeric_var(struct hlsl_ctx *ctx, struct hlsl_ir_var *var,
> -
>    struct parse_initializer *initializer, unsigned int reg_offset, struct hlsl_type *type,
> -
>    unsigned int *initializer_offset)
>
> +{

Isn't "type" here the same thing as "var->data_type" in the only call to 
this function? If so, you could avoid passing "type" explicitly.
> - unsigned int i;
> -
> - if (type->type == HLSL_CLASS_MATRIX)
> -
>    hlsl_fixme(ctx, &var->loc, "Matrix initializer.");
> -
> - for (i = 0; i < type->dimx; i++)
> - {
> -
>    struct hlsl_ir_store *store;
> -
>    struct hlsl_ir_constant *c;
> -
>    struct hlsl_ir_node *node;
> -
> -
>    node = initializer->args[*initializer_offset];
> -
>    *initializer_offset += 1;
> -
> -
>    if (!(node = add_implicit_conversion(ctx, initializer->instrs, node,
> -
>            hlsl_get_scalar_type(ctx, type->base_type), &node->loc)))
> -
>        return;
> -
> -
>    if (hlsl_type_is_single_reg(var->data_type))
> -
>    {
> -
>        if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc)))
> -
>            return;
> -
>        list_add_tail(initializer->instrs, &c->node.entry);
> -
> -
>        if (!(store = hlsl_new_store(ctx, var, &c->node, node, (1u << i), node->loc)))
> -
>            return;
> -
>    }
> -
>    else
> -
>    {
> -
>        if (!(c = hlsl_new_uint_constant(ctx, reg_offset + i, node->loc)))
> -
>            return;
> -
>        list_add_tail(initializer->instrs, &c->node.entry);
> -
> -
>        if (!(store = hlsl_new_store(ctx, var, &c->node, node, 0, node->loc)))
> -
>            return;
> -
>    }

Again under the assumption that "type == var->data_type", here it seems 
that you're handling the matrix case, while it is marked as fixme above. 
I guess the reason for the fixme is that you're only iterating on dimx? 
Still, I find it a bit confusing. If you consider the matrix case not 
(yet) supported, I'd just avoid to add pieces of it until you're really 
supporting it. Though maybe I missing something.

Thanks, Giovanni.