[PATCH vkd3d 07/12] vkd3d-shader/hlsl: Support matrix indexing.

[Zebediah Figura]

On 4/18/22 01:34, Giovanni Mascellani wrote:
> Signed-off-by: Giovanni Mascellani <gmascellani at codeweavers.com>
> ---
>   libs/vkd3d-shader/hlsl.y               | 87 +++++++++++++++++++++++++-
>   tests/hlsl-matrix-indexing.shader_test |  8 +--
>   2 files changed, 88 insertions(+), 7 deletions(-)
> 
> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> index 291f8392..b6a8e496 100644
> --- a/libs/vkd3d-shader/hlsl.y
> +++ b/libs/vkd3d-shader/hlsl.y
> @@ -581,6 +581,89 @@ static bool add_record_load(struct hlsl_ctx *ctx, struct list *instrs, struct hl
>       return !!add_load(ctx, instrs, record, &c->node, field->type, loc);
>   }
>   
> +static struct hlsl_ir_expr *add_binary_arithmetic_expr(struct hlsl_ctx *ctx, struct list *instrs,
> +        enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg1, struct hlsl_ir_node *arg2,
> +        const struct vkd3d_shader_location *loc);
> +
> +static struct hlsl_ir_node *compute_matrix_offset(struct hlsl_ctx *ctx, struct list *instrs,
> +        struct hlsl_type *type, struct hlsl_ir_node *x, struct hlsl_ir_node *y,
> +        const struct vkd3d_shader_location *loc)
> +{
> +    struct hlsl_ir_node *major, *minor;
> +    struct hlsl_ir_expr *mul, *add;
> +    struct hlsl_ir_constant *four;
> +
> +    if (type->modifiers & HLSL_MODIFIER_ROW_MAJOR)
> +    {
> +        minor = x;
> +        major = y;
> +    }
> +    else
> +    {
> +        minor = y;
> +        major = x;
> +    }
> +
> +    if (!(four = hlsl_new_uint_constant(ctx, 4, loc)))
> +        return NULL;
> +    list_add_tail(instrs, &four->node.entry);
> +
> +    if (!(mul = add_binary_arithmetic_expr(ctx, instrs, HLSL_OP2_MUL, &four->node, major, loc)))
> +        return NULL;
> +
> +    if (!(add = add_binary_arithmetic_expr(ctx, instrs, HLSL_OP2_ADD, &mul->node, minor, loc)))
> +        return NULL;

Why do we need this, and not just hlsl_new_binary_expr()? We already 
cast the index to a scalar uint.

In general I think we want to avoid using add_*() helpers if possible, 
where not triggered directly by user code.

> +
> +    return &add->node;
> +}
> +
> +static bool add_matrix_load(struct hlsl_ctx *ctx, struct list *instrs,
> +        struct hlsl_ir_node *matrix, struct hlsl_ir_node *index, const struct vkd3d_shader_location *loc)

How about add_matrix_index() instead?

> +{
> +    struct hlsl_type *mat_type = matrix->data_type, *ret_type, *scal_type;
> +    static unsigned int counter = 0;
> +    struct hlsl_ir_load *load;
> +    struct hlsl_ir_var *var;
> +    unsigned int i;
> +    char name[32];
> +
> +    ret_type = hlsl_get_vector_type(ctx, mat_type->base_type, mat_type->dimx);
> +    scal_type = hlsl_get_scalar_type(ctx, mat_type->base_type);
> +
> +    sprintf(name, "<index-%x>", counter++);

Please always use # for hexadecimal formats (or, in this case, %u may be 
better). I see the constructor path doesn't, but we should change that.

> +    var = hlsl_new_synthetic_var(ctx, name, ret_type, *loc);
> +    if (!var)
> +        return false;
> +
> +    for (i = 0; i < mat_type->dimx; ++i)
> +    {
> +        struct hlsl_ir_node *offset;
> +        struct hlsl_ir_store *store;
> +        struct hlsl_ir_load *value;
> +        struct hlsl_ir_constant *c;
> +
> +        if (!(c = hlsl_new_uint_constant(ctx, i, loc)))
> +            return false;
> +        list_add_tail(instrs, &c->node.entry);
> +
> +        if (!(offset = compute_matrix_offset(ctx, instrs, mat_type, &c->node, index, loc)))
> +            return false;

The signature of this function feels awkward to me. Could we fold the 
subsequent add_load() call into it, and call it something like 
add_matrix_scalar_load() instead? Note also that it'd be useful for 
matrix swizzles in that case, since I think we want to stop generating 
HLSL_IR_SWIZZLE instructions for those.

> +
> +        if (!(value = add_load(ctx, instrs, matrix, offset, scal_type, *loc)))
> +            return false;
> +
> +        if (!(store = hlsl_new_store(ctx, var, &c->node, &value->node, 0, *loc)))
> +            return false;
> +        list_add_tail(instrs, &store->node.entry);
> +    }
> +
> +    if (!(load = hlsl_new_load(ctx, var, NULL, ret_type, *loc)))

hlsl_new_var_load(ctx, var, *loc)

> +        return false;
> +    list_add_tail(instrs, &load->node.entry);
> +
> +    return true;
> +}
> +
>   static bool add_array_load(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *array,
>           struct hlsl_ir_node *index, const struct vkd3d_shader_location loc)
>   {