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

[Francisco Casas]

Hello,

On 19-07-22 05:11, Giovanni Mascellani wrote:
> 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.
> 

I didn't think on Hungarian notation, but I indeed added the 'p' to 
indicate that this is a "pointer to" the actual value.

Because the referenced values (*typep and *indexp) have to be used 
several times, and typep is a double pointer, I assigned these values to 
local variables:

struct hlsl_type *type = *typep;
unsigned int index = *indexp;

Which is my opinion makes it far more readable that constantly using the 
dereference operator.

The problem is that I had to pick a different name for the local 
variables an the function arguments.

But, unless there is another suggestion, in v3 I am renaming the 
pointers "typep" and "indexp" to "type" and "index" respectively, and 
the values from "type" and "index" to "type_val" and "index_val" 
respectively.

>> +{
>> +    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?
> 

I see; changed.

>> +        }
>> +
>> +        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.
> 

Understood, I will prefer present sense and active voice from now on.

The function compute_component_path() will be removed in v3, but I am 
writing the comment in subtype_index_from_component_index() as:

/* Given a type and a component index, this function returns the next
  * path index required to reach the component within the type.
  * It sets *type to the subtype within the original type that contains
  * the component.
  * It sets *index to the index of the component within *type. */

>> +/* 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.
> 

The point is that the derefs retrieved by this function in particular do 
not need to be passed to hlsl_cleanup_deref() afterwards because they 
don't point to allocated memory in the heap.
However, if this deref is modified, this assumption no longer holds true 
(unless only the pointer to the variable is modified, but in that case 
it makes more sense to create another deref).

Is is better to say "There is no reason for modifying this deref." or, 
even better, to say nothing at all. So I am removing that part of the 
comment. It probably made more sense in the previous version of the 
patch where the deref was retrieved as return value.

>> +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.
> 

Thanks,
Francisco