On 4/21/22 04:53, Giovanni Mascellani wrote:
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.
I dunno, I see the logic as not particularly complex...
I'm also slightly concerned that we're generating terrible code too zealously. Simplicity of implementation is valuable, but it means that optimization passes have more busywork to do, and this is one of those cases where I'm not sure it's actually worth 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.
I find naming to be important enough that it's worth the extra clarity even if the argument types are unambiguous.
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.
Sure, it's fine on a separate line too.
I guess not being used to multiple increments is a personal thing for me, so I probably should just drop this comment anyway.