On 4/18/22 01:34, Giovanni Mascellani wrote:
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- 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?