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

[Francisco Casas]

On 15-07-22 17:52, Zebediah Figura wrote:
> On 7/14/22 20:23, Francisco Casas wrote:
>> * The introduction of hlsl_new_store_index() was moved to another patch.
>> * The introduction of hlsl_new_store_component() mas moved to another
>>   patch.
>> * The translation of hlsl_new_resource_load() to register offsets was
>>   split from this patch.
> 
> Yeah, although I kind of meant moving them *before* this patch, along 
> the principle of "abstract away the interface and then change the 
> implementation". Probably not worth rewriting the series again at this 
> point, though.
> 

I see, that sounds useful for reducing the number of diff chunks. 
Albeit, at the cost of having to add an intermediate implementation.
I will try to keep that principle in mind.

>> Regarding the possibility of having a 
>> hlsl_deref_from_component_index() function, there are 2 inconveniences:
>> 1. It is necessary to create constant nodes for the path, so, an 
>> instruction block pointer is required.
>> 2. An additional hlsl_deref pointer is required for its use in 
>> hlsl_new_store_component(), since the lhs path needs to be prepend to 
>> the path to the component [1].
>>
>> The signature of the function would end up like this:
>>
>> static bool hlsl_deref_from_component_index(struct hlsl_ctx *ctx, 
>> struct hlsl_block *block,
>>          struct hlsl_deref *deref, struct hlsl_ir_var *var, unsigned 
>> int index,
>>          struct hlsl_deref *path_prefix, const struct 
>> vkd3d_shader_location *loc)
>>
>> So in the end I think it is not a good idea to have 
>> hlsl_deref_from_component_index() directly.
> 
>> [1] There is also the alternative of introducing a hlsl_deref_concat() 
>> function that takes two derefs and retrieves a new one.
> 
> I don't think we'd need to pass the var *and* deref, would we?
> 

Ah yes, we can use the deref to get the var.

> Also: why do we need to pass a deref pointer to 
> hlsl_new_store_component() in the first place? From 13/17 we only use it 
> with variables directly. I guess the answer might be "consistency with 
> hlsl_new_store_index()", which isn't a bad answer.
> 

Hmm, good point, yes, I don't have a better argument than that.
It may be useful in translating the next passes but I can't think of any 
use case yet.

> But, that said, I don't think that either of these are prohibitive, or 
> make things worse than the alternative.
> 

So, just to confirm, I should replace compute_component_path() with a 
hlsl_deref_from_component_index() in v3. Right?

>> +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.
>> + */
>> +static unsigned int *compute_component_path(struct hlsl_ctx *ctx, 
>> struct hlsl_type *type,
>> +        unsigned int index, unsigned int *path_len)
>> +{
>> +    struct hlsl_type *path_type;
>> +    unsigned int *path, path_index;
>> +
>> +    *path_len = 0;
>> +    path_type = type;
>> +    path_index = index;
>> +    while (!type_is_single_component(path_type))
>> +    {
>> +        subtype_index_from_component_index(ctx, &path_type, 
>> &path_index);
>> +        ++*path_len;
>> +    }
>> +    if (!(path = hlsl_alloc(ctx, *path_len * sizeof(unsigned int) + 1)))
>> +        return NULL;
>> +
>> +    *path_len = 0;
>> +    path_type = type;
>> +    path_index = index;
>> +    while (!type_is_single_component(path_type))
>> +    {
>> +        path[*path_len] = subtype_index_from_component_index(ctx, 
>> &path_type, &path_index);
>> +        ++*path_len;
>> +    }
>> +
>> +    return path;
>> +}
> 
> Could we return path_type from this function, since we've already 
> calculated it, and throw out hlsl_type_get_component_type() entirely?
> 

We would be giving the caller the responsibility of freeing the returned 
path even if it is not used (which I think was a concern in v1). It also 
happens often that the path is required but not the component type.

So, given that the implementation of both functions is small and doesn't 
introduce too much overhead, I think that keeping them separate is 
better, but, as always, if you insist I will change it.

>> @@ -757,13 +948,21 @@ struct hlsl_ir_store *hlsl_new_store(struct 
>> hlsl_ctx *ctx, struct hlsl_ir_var *v
>>           return NULL;
>>       init_node(&store->node, HLSL_IR_STORE, NULL, loc);
>> -    store->lhs.var = var;
>> +    init_deref(ctx, &store->lhs, var, 0);
> 
> Or hlsl_init_simple_deref_from_var(), here and in hlsl_new_load()?
> 
>>       hlsl_src_from_node(&store->lhs.offset, offset);
>>       hlsl_src_from_node(&store->rhs, rhs);
>>       store->writemask = writemask;
>>       return store;
>>   }
>