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

[Giovanni Mascellani]

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?

>> +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().

> 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).

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.

>> +    if (!(load = hlsl_new_load(ctx, var, NULL, ret_type, *loc)))
> 
> hlsl_new_var_load(ctx, var, *loc)

Right, done.

Thanks, Giovanni.