Re: [PATCH vkd3d v3 09/11] vkd3d-shader/hlsl: Support all complex initalizers.

[Francisco Casas]

Hello,

April 22, 2022 6:51 AM, "Giovanni Mascellani"  wrote:
> Hi,
>
> I am not completely in love with the additional runtime complexity of this solution, but it's true
> that it's quite clean.

I have a couple of ideas for lowering the complexity:

a) Add a "component_count" field to hlsl_type in order to store/cache the result of hlsl_type_component_count().
  Which would also speed up other parts of the compiler.

b) Add a "comp_offsets" array to "hlsl_type.e" so that the right field that has a given component
can be selected in hlsl_compute_component_reg_offset() using a binary search.

I can make new patches for these.
> Il 22/04/22 12:25, Giovanni Mascellani ha scritto:
> > +/* Returns the register offset of a given component within a type, given its index.
> > -
> > - *comp_type, will be set to the type of the component, unless it is NULL. */
> >
> > +unsigned int hlsl_compute_component_reg_offset(struct hlsl_ctx *ctx, struct hlsl_type *type,
> > - unsigned int idx, struct hlsl_type **comp_type, const struct vkd3d_shader_location *loc)
>
> Is the "loc" parameter actually used for something, except being passed around?

I see, it is just being passed around here. 
I will remove it.
> Also, personally I'd just write "offset" rather than "reg_offset". It seems to be clear enough.
> This applies also to the variables below.
> > +{
> > - switch (type->type)
> > - {
> > - case HLSL_CLASS_SCALAR:
> > - case HLSL_CLASS_VECTOR:
> > - {
> > - assert(idx < type->dimx * type->dimy);
> > -
> > - if (comp_type)
> > - *comp_type = hlsl_get_scalar_type(ctx, type->base_type);
>
> I think you can assume that comp_type will always be valid and skip this check.

I think hlsl_compute_component_reg_offset() can be used to simplify other functions,
like compute_offset_from_index(), and in those cases it could be much cleaner to call
it passing a NULL comp_type.
> > - return idx;
> > - }
> > - case HLSL_CLASS_MATRIX:
> > - {
> > - unsigned int minor, major, x = idx % type->dimx, y = idx / type->dimx;
> > -
> > - assert(idx < type->dimx * type->dimy);
> > -
> > - if (hlsl_type_is_row_major(type))
> > - {
> > - minor = x;
> > - major = y;
> > - }
> > - else
> > - {
> > - minor = y;
> > - major = x;
> > - }
> > -
> > - if (comp_type)
> > - *comp_type = hlsl_get_scalar_type(ctx, type->base_type);
> > - return 4 * major + minor;
> > - }
> > -
> > - case HLSL_CLASS_ARRAY:
> > - {
> > - unsigned int elem_comp_count = hlsl_type_component_count(type->e.array.type);
> > - unsigned int array_idx = idx / elem_comp_count;
> > - unsigned int idx_in_elem = idx - array_idx * elem_comp_count;
>
> BTW, here you are just computing a modulus, you could directlyuse "%". Addressing arrays is not
> fundamentally different than addressing matrices, except you don't have to decide which one is
> major and which one is minor.

Okay, I will write it as
unsigned int idx_in_elem = idx % elem_comp_count;
> > - assert(array_idx < type->e.array.elements_count);
> > -
> > - return array_idx * hlsl_type_get_array_element_reg_size(type->e.array.type) +
> > - hlsl_compute_component_reg_offset(ctx, type->e.array.type, idx_in_elem, comp_type, loc);
> > - }
> > -
> > - case HLSL_CLASS_STRUCT:
> > - {
> > - struct hlsl_struct_field *field;
> > -
> > - LIST_FOR_EACH_ENTRY(field, type->e.elements, struct hlsl_struct_field, entry)
> > - {
> > - unsigned int elem_comp_count = hlsl_type_component_count(field->type);
> > -
> > - if (idx < elem_comp_count)
> > - {
> > - return field->reg_offset +
> > - hlsl_compute_component_reg_offset(ctx, field->type, idx, comp_type, loc);
> > - }
> > - idx -= elem_comp_count;
> > - }
> > -
> > - assert(0);
> > - return 0;
> > - }
> > -
> > - case HLSL_CLASS_OBJECT:
> > - {
> > - assert(idx == 0);
> > - if (comp_type)
> > - *comp_type = type;
> > - return 0;
> > - }
> > - }
> > -
> > - assert(0);
> > - return 0;
> >
> > +}
> > -
>
> Thanks, Giovanni.