Hi,
Il 20/04/22 20:47, Zebediah Figura ha scritto:
+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()...]
That would require to duplicate the logic behind compute_matrix_offset() (once for computing in the compiler, one for computing in the shader). Granted, it's not a particularly complicated logic, but still I'd prefer to keep one copy of it.
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.
To me that's sort of obvious, given that we're passing the instruction list as a parameter, but I can do that. I suppose that would apply to compute_matrix_offset() too, then.
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.
OTOH, I don't like very much when operators mutating data is buried inside a larger formula, except when it happens at very idiomatic positions, like "x[i++] = ..." or "*ptr++ = ...", or of course in the third slot of a "for" statement. "x = y = z" is already something that hurts legibility a lot, IMHO.
But I can live with your suggestion, so I'll do it.
{ - 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?
Ok.
Giovanni.