[PATCH vkd3d 09/17] vkd3d-shader/hlsl: Replace register offsets with index paths in load initializations.

[Giovanni Mascellani]

Hi,

Il 15/07/22 03:23, Francisco Casas ha scritto:
> +/* Given a type and a component index, retrieves next path index required to reach the component.
> + * *typep will be set to the subtype within the original type that contains the component.
> + * *indexp will be set to the index of the component within *typep.
> + */
> +static unsigned int subtype_index_from_component_index(struct hlsl_ctx *ctx,
> +        struct hlsl_type **typep, unsigned int *indexp)

I guess that the "p"'s in "typep" and "indexp" are a sort of reverse 
Hungarian notation. It's a nitpick, but I am not really a fan of that, 
and I don't think we're using that anywhere in the HLSL compiler.

> +{
> +    struct hlsl_type *type = *typep;
> +    unsigned int index = *indexp;
> +
> +    assert(!type_is_single_component(type));
> +    assert(index < hlsl_type_component_count(type));
> +
> +    switch (type->type)
> +    {
> +        case HLSL_CLASS_VECTOR:
> +            assert(index < type->dimx);
> +            *typep = hlsl_get_scalar_type(ctx, type->base_type);
> +            *indexp = 0;
> +            return index;
> +
> +        case HLSL_CLASS_MATRIX:
> +        {
> +            unsigned int y = index / type->dimx, x = index % type->dimx;
> +            bool row_major = hlsl_type_is_row_major(type);
> +
> +            assert(index < type->dimx * type->dimy);
> +            *typep = hlsl_get_vector_type(ctx, type->base_type, row_major? type->dimx : type->dimy);
> +            *indexp = row_major? x : y;
> +            return row_major? y : x;

I think we leave a space before the operator "?", don't we?

> +        }
> +
> +        case HLSL_CLASS_ARRAY:
> +        {
> +            unsigned int elem_comp_count = hlsl_type_component_count(type->e.array.type);
> +            unsigned int array_index;
> +
> +            *typep = type->e.array.type;
> +            *indexp = index % elem_comp_count;
> +            array_index = index / elem_comp_count;
> +            assert(array_index < type->e.array.elements_count);
> +            return array_index;
> +        }
> +
> +        case HLSL_CLASS_STRUCT:
> +        {
> +            struct hlsl_struct_field *field;
> +            unsigned int field_comp_count, i;
> +
> +            for (i = 0; i < type->e.record.field_count; ++i)
> +            {
> +                field = &type->e.record.fields[i];
> +                field_comp_count = hlsl_type_component_count(field->type);
> +                if (index < field_comp_count)
> +                {
> +                    *typep = field->type;
> +                    *indexp = index;
> +                    return i;
> +                }
> +                index -= field_comp_count;
> +            }
> +            assert(0);
> +            return 0;
> +        }
> +
> +        default:
> +            assert(0);
> +            return 0;
> +    }
> +}
> +
> +struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, struct hlsl_type *type,
> +        unsigned int index)
> +{
> +    while (!type_is_single_component(type))
> +        subtype_index_from_component_index(ctx, &type, &index);
> +
> +    return type;
> +}
> +
> +/* Returns the path of a given component within a type, given its index.
> + * *path_len will be set to the lenght of the path.
> + * Memory should be free afterwards.
> + */

I find the verbal tenses rather confusing: here we have a present tense, 
a future tense with "will" and a "should". Combined with the typo in 
"free" (I guess that was "freed"?), it makes the whole lot rather hard 
to understand if you don't already know what it means.

I would suggest to describe what this function does using consistently 
either an imperative form or a present tense, and to describe 
expectations from the caller by clearly marking them as such.

For example: "Return the path of a given component within a type, given 
its index, and set *path_len to the length of the path. The caller is 
responsible for freeing the returned pointer with hlsl_free()".

(Many technical writing indications suggest to avoid passive voices to 
make phrasing clearer; I am not sure that should be a general rule, but 
it can be a point to keep in mind)

BTW, the same suggestion applies to the comment before 
subtype_index_from_component_index(), though in that case the potential 
for confusion is smaller.

> +/* Initializes a simple variable derefence, so that a pointer to it can be passed to load/store
> + * functions. The deref shall not be modified afterwards. */

Maybe I am missing something, but why this deref shouldn't be modified? 
Or, maybe, why is it particulatly important to specify that here? Maybe 
the comment could be make more explicit on what is special with this 
function.

> +void hlsl_init_simple_deref_from_var(struct hlsl_deref *deref, struct hlsl_ir_var *var)
> +{
> +    memset(deref, 0, sizeof(*deref));
> +    deref->var = var;
> +}
> +
Thanks, Giovanni.