Re: [PATCH vkd3d 06/12] vkd3d-shader/hlsl: Split matrix copies.

[Matteo Bruni]

On Mon, Apr 18, 2022 at 8:34 AM Giovanni Mascellani
 wrote:
> Signed-off-by: Giovanni Mascellani
>
> libs/vkd3d-shader/hlsl_codegen.c       | 52 ++++++++++++++++++++++++++
>  tests/hlsl-matrix-indexing.shader_test | 17 +++++++++
>  2 files changed, 69 insertions(+)
>
> diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> index 72c00430..73e3b73f 100644
> --- a/libs/vkd3d-shader/hlsl_codegen.c
> +++ b/libs/vkd3d-shader/hlsl_codegen.c
> @@ -672,6 +672,57 @@ static bool split_struct_copies(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr
>      return true;
>  }
>
> +static unsigned int minor_size(const struct hlsl_type *type)
> +{
> - if (type->type == HLSL_CLASS_VECTOR || type->modifiers & HLSL_MODIFIER_ROW_MAJOR)
> -
>    return type->dimx;
> - else
> -
>    return type->dimy;
>
> +}
> -
>
> +static unsigned int major_size(const struct hlsl_type *type)
> +{
> - if (type->type == HLSL_CLASS_VECTOR || type->modifiers & HLSL_MODIFIER_ROW_MAJOR)
> -
>    return type->dimy;
> - else
> -
>    return type->dimx;
>
> +}
> -
>
> +static bool split_matrix_copies(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context)
> +{
> - const struct hlsl_ir_node *rhs;
> - struct hlsl_type *element_type;
> - const struct hlsl_type *type;
> - unsigned int i;
> - struct hlsl_ir_store *store;
> -
> - if (instr->type != HLSL_IR_STORE)
> -
>    return false;
> -
> - store = hlsl_ir_store(instr);
> - rhs = store->rhs.node;
> - type = rhs->data_type;
> - if (type->type != HLSL_CLASS_MATRIX)
> -
>    return false;
> - element_type = hlsl_get_vector_type(ctx, type->base_type, minor_size(type));
> -
> - if (rhs->type != HLSL_IR_LOAD)
> - {
> -
>    hlsl_fixme(ctx, &instr->loc, "Copying from unsupported node type.\n");
> -
>    return false;
> - }
> -
> - for (i = 0; i < major_size(type); ++i)
> - {
> -
>    if (!split_copy(ctx, store, hlsl_ir_load(rhs), 4 * i, element_type))
> -
>        return false;
> - }

I was a bit confused by the major_size() / minor_size() naming,
together with the coordinate and dimension flipping that we always do
with matrices, but this seems to be correct.

It might be just a "me" thing but maybe names like vector_size() and
vector_count() would be potentially more obvious?

We should probably update the comment above split_copy() to include
split_matrix_copies(). Or maybe replace the first sentence altogether
with "Copy an element of a complex variable." or something.
> -
> - list_remove(&store->node.entry);
> - hlsl_free_instr(&store->node);
> - return true;
>
> +}
> -
>
> static bool lower_narrowing_casts(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context)
>  {
>      const struct hlsl_type *src_type, *dst_type;
> @@ -1685,6 +1736,7 @@ int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry
>          progress |= transform_ir(ctx, split_struct_copies, body, NULL);
>      }
>      while (progress);
> - transform_ir(ctx, split_matrix_copies, body, NULL);
> transform_ir(ctx, lower_narrowing_casts, body, NULL);
> transform_ir(ctx, lower_casts_to_bool, body, NULL);
> do
>
> diff --git a/tests/hlsl-matrix-indexing.shader_test b/tests/hlsl-matrix-indexing.shader_test
> index 1d444ffa..e2103fe2 100644
> --- a/tests/hlsl-matrix-indexing.shader_test
> +++ b/tests/hlsl-matrix-indexing.shader_test
> @@ -45,3 +45,20 @@ uniform 8 float4 9.0 10.0 11.0 12.0
>  uniform 12 float4 13.0 14.0 15.0 16.0
>  todo draw quad
>  probe all rgba (1.0, 5.0, 7.0, 12.0)
> -
>
> +[pixel shader]
> +uniform float4x4 m;
> -
>
> +float4 main() : SV_TARGET
> +{
> - float4x4 m2 = m;
> - return float4(m2[0][0], m2[1][0], m2[1][2], m2[2][3]);
>
> +}
> -
>
> +[test]
> +uniform 0 float4 1.0 2.0 3.0 4.0
> +uniform 4 float4 5.0 6.0 7.0 8.0
> +uniform 8 float4 9.0 10.0 11.0 12.0
> +uniform 12 float4 13.0 14.0 15.0 16.0
> +todo draw quad
> +probe all rgba (1.0, 2.0, 10.0, 15.0)

Adding a test with a non-square matrix might be a good idea, if
anything to give a bit more reassurance that we're using the right
dimensions.