Re: [PATCH vkd3d 08/12] vkd3d-shader/hlsl: Parse matrix constructors.

[Zebediah Figura]

On 4/18/22 01:34, Giovanni Mascellani wrote:
> Signed-off-by: Giovanni Mascellani
>
> libs/vkd3d-shader/hlsl.y                      | 98 ++++++++++++++++---
>   tests/hlsl-matrix-indexing.shader_test        | 14 +++
>   ...lsl-return-implicit-conversion.shader_test |  8 +-
>   3 files changed, 100 insertions(+), 20 deletions(-)
>
> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> index b6a8e496..1ee1db4c 100644
> --- a/libs/vkd3d-shader/hlsl.y
> +++ b/libs/vkd3d-shader/hlsl.y
> @@ -2081,18 +2081,87 @@ static struct list *add_call(struct hlsl_ctx *ctx, const char *name,
>       return params->instrs;
>   }
>
> +static struct hlsl_ir_node *compute_offset_from_index(struct hlsl_ctx *ctx, struct list *instrs,
> -
>    struct hlsl_type *type, unsigned int idx, const struct vkd3d_shader_location *loc)
>
> +{
> - assert(type->type <= HLSL_CLASS_LAST_NUMERIC);
> - assert(idx < type->dimx * type->dimy);
> -
> - if (type->type == HLSL_CLASS_SCALAR || type->type == HLSL_CLASS_VECTOR)
> - {
> -
>    struct hlsl_ir_constant *c;
> -
> -
>    if (!(c = hlsl_new_uint_constant(ctx, idx, loc)))
> -
>        return NULL;
> -
>    list_add_tail(instrs, &c->node.entry);
> -
> -
>    return &c->node;
> - }
> - else
> - {
> -
>    struct hlsl_ir_constant *x, *y;
> -
> -
>    if (!(x = hlsl_new_uint_constant(ctx, idx % type->dimx, loc)))
> -
>        return NULL;
> -
>    list_add_tail(instrs, &x->node.entry);
> -
> -
>    if (!(y = hlsl_new_uint_constant(ctx, idx / type->dimx, loc)))
> -
>        return NULL;
> -
>    list_add_tail(instrs, &y->node.entry);
> -
> -
>    return compute_matrix_offset(ctx, instrs, type, &x->node, &y->node, loc);
> - }
>
> +}

I guess, but have you considered just returning an unsigned int from 
this function? It seems like potentially less work [and wouldn't 
invalidate my comment on compute_matrix_offset()...]

If not, I'd prefer to name this function something that makes it obvious 
that it's modifying the instruction stream (i.e. it should begin with 
add_*, probably). That goes for load_index() and store_index() as well.
> -
>
> +static struct hlsl_ir_node *load_index(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *instr,
> -
>    unsigned int idx, const struct vkd3d_shader_location *loc)
>
> +{
> - struct hlsl_type *scal_type = hlsl_get_scalar_type(ctx, instr->data_type->base_type);
> - struct hlsl_ir_node *offset;
> - struct hlsl_ir_load *load;
> -
> - if (instr->data_type->type > HLSL_CLASS_LAST_NUMERIC)
> - {
> -
>    hlsl_fixme(ctx, loc, "Loading from non-numeric type.");
> -
>    return NULL;
> - }
> -
> - if (!(offset = compute_offset_from_index(ctx, instrs, instr->data_type, idx, loc)))
> -
>    return NULL;
> -
> - if (!(load = add_load(ctx, instrs, instr, offset, scal_type, *loc)))
> -
>    return NULL;
> -
> - return &load->node;
>
> +}
> -
>
> +static struct hlsl_ir_node *store_index(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_var *var,
> -
>    struct hlsl_ir_node *rhs, unsigned int idx, const struct vkd3d_shader_location *loc)
>
> +{
> - struct hlsl_ir_node *offset;
> - struct hlsl_ir_store *store;
> -
> - if (!(offset = compute_offset_from_index(ctx, instrs, var->data_type, idx, loc)))
> -
>    return NULL;
> -
> - if (!(store = hlsl_new_store(ctx, var, offset, rhs, 0, *loc)))
> -
>    return NULL;
> - list_add_tail(instrs, &store->node.entry);
> -
> - return &store->node;
>
> +}
> - static struct list *add_constructor(struct hlsl_ctx *ctx, struct hlsl_type *type,
>         struct parse_initializer *params, struct vkd3d_shader_location loc)
> {
> - unsigned int i, writemask_offset = 0;
> - struct hlsl_ir_store *store;
>  static unsigned int counter;
> - struct hlsl_type *scal_type;
> - unsigned int i, j, idx = 0;
>  struct hlsl_ir_load *load;
>  struct hlsl_ir_var *var;
>  char name[23];
> - if (type->type == HLSL_CLASS_MATRIX)
> -
>    hlsl_fixme(ctx, &loc, "Matrix constructor.");
> -
>
> scal_type = hlsl_get_scalar_type(ctx, type->base_type);
>
> sprintf(name, "<constructor-%x>", counter++);
>  if (!(var = hlsl_new_synthetic_var(ctx, name, type, loc)))
>
> @@ -2115,22 +2184,19 @@ static struct list *add_constructor(struct hlsl_ctx *ctx, struct hlsl_type *type
>           }
>           width = hlsl_type_component_count(arg->data_type);
> -
>    if (width > 4)
> -
>    for (j = 0; j < width; ++j, ++idx)

I know this is a common construction, but it always throws me off. I can 
live with it, but I'd prefer to put the ++idx in the store_index() call.
>       {
> -
>        FIXME("Constructor argument with %u components.\n", width);
> -
>        continue;
> -
>    }
> -
>        struct hlsl_ir_node *value;
>
> -
>    if (!(arg = add_implicit_conversion(ctx, params->instrs, arg,
> -
>            hlsl_get_vector_type(ctx, type->base_type, width), &arg->loc)))
> -
>        continue;
> -
>        if (!(value = load_index(ctx, params->instrs, arg, j, &loc)))
> -
>            return NULL;
>
> -
>    if (!(store = hlsl_new_store(ctx, var, NULL, arg,
> -
>            ((1u << width) - 1) << writemask_offset, arg->loc)))
> -
>        return NULL;
> -
>    list_add_tail(params->instrs, &store->node.entry);
> -
>        if (!(value = add_implicit_conversion(ctx, params->instrs, value, scal_type, &arg->loc)))
> -
>            return NULL;
>
> -
>    writemask_offset += width;
> -
>        if (!store_index(ctx, params->instrs, var, value, idx, &loc))
> -
>            return NULL;
> -
>    }
> }
>
> if (!(load = hlsl_new_var_load(ctx, var, loc)))

This can be follow-up patches, but as long as you're implementing it 
here, would you mind adding tests for constructors containing vectors 
and matrices?