[PATCH vkd3d 5/5] vkd3d-shader/hlsl: Correctly calculate offsets for array elements.

[Matteo Bruni]

On Fri, Feb 18, 2022 at 12:05 AM Francisco Casas <fcasas at codeweavers.com> wrote:
>
> Hi,
>
> The failing tests aside, this padding rule for array elements is
> an interesting find.
>
> If this is correct, wouldn't that mean that we also have to use
> hlsl_type_get_array_element_reg_size() in hlsl_type_calculate_reg_size()?
>
> In particular, here
> ---
>         case HLSL_CLASS_ARRAY:
>         {
>             unsigned int element_size = type->e.array.type->reg_size;
>
>             assert(element_size);
>             if (is_sm4)
>                 type->reg_size = (type->e.array.elements_count - 1) * align(element_size, 4) + element_size;
>             else
>                 type->reg_size = type->e.array.elements_count * element_size;
>             break;
>         }
> ---
>
> Also, it makes me wonder whether this rule only applies in sm4 and/or to uniform
> arrays (I don't know a quick way to test that).

Interestingly, this is the code generated for
hlsl-struct-array.shader_test by native for ps_2_0:

    ps_2_0
    mov r0.x, c1.x
    mov r0.y, c2.x
    mov r0.z, c5.x
    mov r0.w, c4.x
    mov oC0, r0

i.e. a full 4-component register is allocated for each struct element.
I seem to recall having seen this before, we might even have tests for
it?

Anyway, you make all valid points in this reply. I guess we could add
more tests and then fix stuff accordingly in follow up patches, or get
a better picture first and then implement it correctly right away. I'm
resending the patch with minimal changes, since I have it here anyway,
but (all of you) feel free to sign-off or not.

> If I am not mistaken and we indeed have to change hlsl_type_calculate_reg_size(),
> then we would have to also take into account the padding in the
> HLSL_CLASS_STRUCT case, when the struct contains array fields.
>
>
> Thanks,
> Francisco.
>
>
> February 16, 2022 2:47 AM, "Zebediah Figura" <zfigura at codeweavers.com> wrote:
>
> > Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
> > ---
> > Makefile.am | 2 +-
> > libs/vkd3d-shader/hlsl.c | 7 +++++++
> > libs/vkd3d-shader/hlsl.h | 1 +
> > libs/vkd3d-shader/hlsl.y | 2 +-
> > libs/vkd3d-shader/hlsl_codegen.c | 2 +-
> > tests/hlsl-struct-array.shader_test | 19 +++++++++++++++++++
> > 6 files changed, 30 insertions(+), 3 deletions(-)
> > create mode 100644 tests/hlsl-struct-array.shader_test
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 75c5e4b1..74e3603c 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -93,6 +93,7 @@ vkd3d_shader_tests = \
> > tests/hlsl-state-block-syntax.shader_test \
> > tests/hlsl-static-initializer.shader_test \
> > tests/hlsl-storage-qualifiers.shader_test \
> > + tests/hlsl-struct-array.shader_test \
> > tests/hlsl-struct-assignment.shader_test \
> > tests/hlsl-struct-semantics.shader_test \
> > tests/hlsl-vector-indexing.shader_test \
> > @@ -326,7 +327,6 @@ XFAIL_TESTS = \
> > tests/hlsl-majority-pragma.shader_test \
> > tests/hlsl-majority-typedef.shader_test \
> > tests/hlsl-mul.shader_test \
> > - tests/hlsl-nested-arrays.shader_test \
> > tests/hlsl-numeric-constructor-truncation.shader_test \
> > tests/hlsl-numeric-types.shader_test \
> > tests/hlsl-operations.shader_test \
> > diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
> > index 8a0f4b01..9c511018 100644
> > --- a/libs/vkd3d-shader/hlsl.c
> > +++ b/libs/vkd3d-shader/hlsl.c
> > @@ -202,6 +202,13 @@ static void hlsl_type_calculate_reg_size(struct hlsl_ctx *ctx, struct
> > hlsl_type
> > }
> > }
> >
> > +/* Returns the size of a type, considered as part of an array of that type.
> > + * As such it includes padding after the type. */
> > +unsigned int hlsl_type_get_array_element_reg_size(const struct hlsl_type *type)
> > +{
> > + return align(type->reg_size, 4);
> > +}
> > +
> > static struct hlsl_type *hlsl_new_type(struct hlsl_ctx *ctx, const char *name, enum hlsl_type_class
> > type_class,
> > enum hlsl_base_type base_type, unsigned dimx, unsigned dimy)
> > {
> > diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
> > index 9b1c9ad3..8ac3e0c5 100644
> > --- a/libs/vkd3d-shader/hlsl.h
> > +++ b/libs/vkd3d-shader/hlsl.h
> > @@ -780,6 +780,7 @@ bool hlsl_scope_add_type(struct hlsl_scope *scope, struct hlsl_type *type);
> > struct hlsl_type *hlsl_type_clone(struct hlsl_ctx *ctx, struct hlsl_type *old,
> > unsigned int default_majority, unsigned int modifiers);
> > unsigned int hlsl_type_component_count(struct hlsl_type *type);
> > +unsigned int hlsl_type_get_array_element_reg_size(const struct hlsl_type *type);
> > unsigned int hlsl_type_get_sm4_offset(const struct hlsl_type *type, unsigned int offset);
> > bool hlsl_types_are_equal(const struct hlsl_type *t1, const struct hlsl_type *t2);
> >
> > diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> > index 459ad03b..70109780 100644
> > --- a/libs/vkd3d-shader/hlsl.y
> > +++ b/libs/vkd3d-shader/hlsl.y
> > @@ -607,7 +607,7 @@ static struct hlsl_ir_load *add_array_load(struct hlsl_ctx *ctx, struct list
> > *in
> > return NULL;
> > }
> >
> > - if (!(c = hlsl_new_uint_constant(ctx, data_type->reg_size, loc)))
> > + if (!(c = hlsl_new_uint_constant(ctx, hlsl_type_get_array_element_reg_size(data_type), loc)))
> > return NULL;
> > list_add_tail(instrs, &c->node.entry);
> > if (!(mul = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, index, &c->node)))
> > diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> > index 25f038d4..ccc95c90 100644
> > --- a/libs/vkd3d-shader/hlsl_codegen.c
> > +++ b/libs/vkd3d-shader/hlsl_codegen.c
> > @@ -634,7 +634,7 @@ static bool split_array_copies(struct hlsl_ctx *ctx, struct hlsl_ir_node
> > *instr,
> > if (type->type != HLSL_CLASS_ARRAY)
> > return false;
> > element_type = type->e.array.type;
> > - element_size = element_type->reg_size;
> > + element_size = hlsl_type_get_array_element_reg_size(element_type);
> >
> > for (i = 0; i < type->e.array.elements_count; ++i)
> > {
> > diff --git a/tests/hlsl-struct-array.shader_test b/tests/hlsl-struct-array.shader_test
> > new file mode 100644
> > index 00000000..01996a50
> > --- /dev/null
> > +++ b/tests/hlsl-struct-array.shader_test
> > @@ -0,0 +1,19 @@
> > +% In SM4, arrays are always aligned to the next vec4, although structs are not.
> > +
> > +[pixel shader]
> > +uniform struct
> > +{
> > + float p, q;
> > +} colours[3];
> > +
> > +float4 main() : sv_target
> > +{
> > + return float4(colours[0].q, colours[1].p, colours[2].q, colours[2].p);
> > +}
> > +
> > +[test]
> > +uniform 0 float4 0.1 0.2 0.0 0.0
> > +uniform 4 float4 0.3 0.4 0.0 0.0
> > +uniform 8 float4 0.5 0.6 0.0 0.0
> > +draw quad
> > +probe all rgba (0.2, 0.3, 0.6, 0.5)
> > --
> > 2.34.1
>