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

[Zebediah Figura]

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.