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

[Francisco Casas]

Hello,

April 22, 2022 6:51 AM, "Giovanni Mascellani" <gmascellani at codeweavers.com> 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.