On 4/21/22 04:22, Giovanni Mascellani wrote:
Hi,
Il 20/04/22 20:28, Zebediah Figura ha scritto:
+ 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.
It's convenient because I don't have to manually add the new instruction to the list.
In general I think we want to avoid using add_*() helpers if possible, where not triggered directly by user code.
Why?
The intent is that they contain most code corresponding to user actions, and as such do nontrivial interpretation and validation. In the case where we're manually synthesizing instructions I don't want to have to think about whether such functions are doing something we don't want.
I can probably live with it in this case, but I'm concerned that this mental burden will grow.
Maybe there's an argument for helpers which just wrap hlsl_new_* like that, and we can have an add_* family and a parse_* family.
+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?
I can do that if you really insist, but given that we already have add_record_load() and add_array_load() it seems sensible to name this add_matrix_load().
In isolation I guess it's fine, but it came to mind while thinking about add_matrix_scalar_load()—it'd be nice to clarify how this function is different from that one.
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.
Ok, I changed to %u.
+ 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.
In "Parse matrix constructors." that function is used in other contexts, for which a subsequent load doesn't make sense (e.g., because you have to store rather than load). So I would still need a helper that just computes the offset and returns it to avoid code duplication. Once we have that, adding another helper to just add the load seems excessive to me (the case is different for load_index() and store_index() in "Parse matrix constructors.", because I already know that other sites will use them in subsequent patches).
Well, right, but I'm not sure it's worth organizing patch 8/12 like that, as I say :-)
WRT your comment about not generating HLSL_IR_SWIZZLE instructions, you mean that instead of
trace:hlsl_dump_function: 2: uint | 8 trace:hlsl_dump_function: 3: float4 | m[@2] trace:hlsl_dump_function: 4: float1 | @3.y
we'd want
trace:hlsl_dump_function: 2: uint | 9 trace:hlsl_dump_function: 3: float4 | m[@2]
directly?
If so, I can agree, but this seems something more easily handled in a later optimization pass, isn't it? The current architecture wouldn't allow for that easily any way, because when you're parsing the first subscript in an expression like "m[0][1]" you don't know yet if there is going to be a second one, or you just genuinely want a vector expression.
I'm referring to 'x._m00_m12_m33' swizzles, rather. We currently encode the whole swizzle in the hlsl_ir_swizzle string, the same way we do for vectors, but I suspect we don't want to do that.