[PATCH vkd3d 07/12] vkd3d-shader/hlsl: Replace register offsets with index paths in parse code.

[Francisco Casas]

On 08-07-22 17:28, Zebediah Figura wrote:
> On 7/1/22 16:24, Francisco Casas wrote:
>> +/* Returns the path of a given component within a type, given its index.
>> + * *comp_type will be set to the type of the component.
>> + * *path_len will be set to the lenght of the path.
>> + * Memory should be free afterwards.
>> + */
>> +unsigned int *hlsl_compute_component_path(struct hlsl_ctx *ctx, 
>> struct hlsl_type *type,
>> +        unsigned int idx, struct hlsl_type **comp_type, unsigned int 
>> *path_len)
> 
> This is awkward partly because zero is a valid length. Ideally it should 
> return bool. More than that, though, it's filling fields of struct 
> hlsl_deref, so what about something like
> 
> static bool hlsl_deref_from_component_index(struct hlsl_ctx *ctx,
>          struct hlsl_deref *deref, struct hlsl_ir_var *var,
>          unsigned int index)
> 
> This obviously doesn't lend itself nicely to recursion. I ended up 
> playing around with this for a bit and came up with the following 
> abbreviated code:
> 
> unsigned int subtype_index_from_component_index(struct hlsl_ctx *ctx,
>          inout const struct hlsl_type **type, inout unsigned int *index)
> {
>      assert(index < hlsl_type_component_count(type));
> 
>      ...
> }
> 
> static bool hlsl_deref_from_component_index(struct hlsl_ctx *ctx,
>          struct hlsl_deref *deref, struct hlsl_ir_var *var, uint index)
> {
>      const struct hlsl_type *type = var->data_type;
>      unsigned int *indices;
>      size_t index_count;
> 
>      while (/* type is not 1x1 numeric */)
>      {
>          indices[index_count++]
>                  = subtype_index_from_component(ctx, &type, &index);
>      }
> 
>      init_deref(ctx, deref, var, index_count);
>      // and so on
> }
> 
> Thoughts?
> 

At first glance, and assuming we don't need 
hlsl_compute_component_path() somewhere else, this seems like a great idea!

I could also use subtype_index_from_component_index() to create a 
function to retrieve the component type without retrieving the path. As 
this is needed a couple of times.

>> +/* Returns a simple variable derefence, so that the value can be 
>> stored and then passed by reference
>> + * to load/store functions. It shall not be modified afterwards. */
>> +struct hlsl_deref hlsl_get_direct_var_deref(struct hlsl_ir_var *var)
>> +{
>> +    struct hlsl_deref deref = {};
>> +
>> +    deref.var = var;
>> +    return deref;
>> +}
>> +
> 
> Naming: for consistency with hlsl_new_simple_store() I'd replace 
> "direct" with "simple". (Also naming: perhaps hlsl_simple_deref_from_var 
> instead, i.e. follow the "X from Y" pattern given what the function is 
> doing).
> 
> I'm also inclined to say this should take the hlsl_deref as a pointer in 
> the first argument, rather than returning a struct.
> 

Okay.

>>  struct hlsl_ir_resource_load *hlsl_new_resource_load(struct hlsl_ctx 
>> *ctx, struct hlsl_type *data_type,
>> -        enum hlsl_resource_load_type type, struct hlsl_ir_var 
>> *resource, struct hlsl_ir_node *resource_offset,
>> -        struct hlsl_ir_var *sampler, struct hlsl_ir_node 
>> *sampler_offset, struct hlsl_ir_node *coords,
>> -        struct hlsl_ir_node *texel_offset, const struct 
>> vkd3d_shader_location *loc)
>> +        enum hlsl_resource_load_type type, struct hlsl_deref 
>> *resource, struct hlsl_deref *sampler,
>> +        struct hlsl_ir_node *coords, struct hlsl_ir_node 
>> *texel_offset, const struct vkd3d_shader_location *loc)
> 
> Separate patch?
> 

Hmm, I will give it a try.

>> @@ -870,10 +1136,8 @@ struct hlsl_ir_resource_load 
>> *hlsl_new_resource_load(struct hlsl_ctx *ctx, struc
>>           return NULL;
>>       init_node(&load->node, HLSL_IR_RESOURCE_LOAD, data_type, *loc);
>>       load->load_type = type;
>> -    load->resource.var = resource;
>> -    hlsl_src_from_node(&load->resource.offset, resource_offset);
>> -    load->sampler.var = sampler;
>> -    hlsl_src_from_node(&load->sampler.offset, sampler_offset);
>> +    deref_copy(ctx, &load->resource, resource);
>> +    deref_copy(ctx, &load->sampler, sampler);
> 
> Actually that means deref_copy() could be pulled out of this patch, 
> right? Just to save another hunk :-)
> 

Sure!

>> @@ -334,7 +336,10 @@ static struct hlsl_ir_node *add_cast(struct 
>> hlsl_ctx *ctx, struct list *instrs,
>>                   }
>>               }
>> -            dst_offset = hlsl_compute_component_offset(ctx, dst_type, 
>> dst_idx, &dst_scalar_type);
>> +            dst_path = hlsl_compute_component_path(ctx, dst_type, 
>> dst_idx, &dst_scalar_type, &dst_path_len);
>> +            if (dst_path_len && !dst_path)
>> +                return NULL;
>> +            vkd3d_free(dst_path);
> 
> As far as I can tell we're just doing this so that we can get the right 
> destination type to cast into. In that case my inclination is to move 
> that cast into hlsl_new_store_component() itself. Same in 
> initialize_var_components().
> 

We could also use the aforementioned hypothetical function that 
retrieves the component type without retrieving the path. I will 
evaluate both alternatives.

>>               if (!(load = add_load_component(ctx, instrs, node, 
>> src_idx, *loc)))
>>                   return NULL;
>> @@ -343,16 +348,12 @@ static struct hlsl_ir_node *add_cast(struct 
>> hlsl_ctx *ctx, struct list *instrs,
>>                   return NULL;
>>               list_add_tail(instrs, &cast->node.entry);
>> -            if (!(c = hlsl_new_uint_constant(ctx, dst_offset, loc)))
>> +            if (!(store = hlsl_new_store_component(ctx, &block, 
>> &var_deref, dst_idx, &cast->node)))
>>                   return NULL;
>> -            list_add_tail(instrs, &c->node.entry);
>> -
>> -            if (!(store = hlsl_new_store(ctx, var, &c->node, 
>> &cast->node, 0, *loc)))
>> -                return NULL;
>> -            list_add_tail(instrs, &store->node.entry);
>> +            list_move_tail(instrs, &block.instrs);
>>           }
>> -        if (!(load = hlsl_new_load(ctx, var, NULL, dst_type, *loc)))
>> +        if (!(load = hlsl_new_var_load(ctx, var, *loc)))
>>               return NULL;
>>           list_add_tail(instrs, &load->node.entry);
> 
> ...
> 
>> @@ -625,22 +626,13 @@ static struct hlsl_ir_jump *add_return(struct 
>> hlsl_ctx *ctx, struct list *instrs
>>   static struct hlsl_ir_load *add_load_index(struct hlsl_ctx *ctx, 
>> struct list *instrs, struct hlsl_ir_node *var_node,
>>           struct hlsl_ir_node *idx, const struct vkd3d_shader_location 
>> loc)
>>   {
>> -    struct hlsl_type *elem_type;
>> -    struct hlsl_ir_node *offset;
>>       struct hlsl_ir_load *load;
>> -    struct hlsl_block block;
>> -
>> -    elem_type = hlsl_get_type_from_path_index(ctx, 
>> var_node->data_type, idx);
>>       if (var_node->type == HLSL_IR_LOAD)
>>       {
>>           const struct hlsl_deref *src = &hlsl_ir_load(var_node)->src;
>> -        if (!(offset = hlsl_new_offset_from_path_index(ctx, &block, 
>> var_node->data_type, src->offset.node, idx, &loc)))
>> -            return NULL;
>> -        list_move_tail(instrs, &block.instrs);
>> -
>> -        if (!(load = hlsl_new_load(ctx, src->var, offset, elem_type, 
>> loc)))
>> +        if (!(load = hlsl_new_load_index(ctx, src, idx, loc)))
>>               return NULL;
>>           list_add_tail(instrs, &load->node.entry);
>>       }
> 
> Is it possible to introduce hlsl_new_load_index() and 
> hlsl_new_load_component() in helper patch(es), the same way we did for 
> add_load_index()? Same for stores I think.
> 
> (I know this is a lot of splitting, and/or busywork, but the more the 
> patch is split, the easier it is to be sure we've caught all the errors.)
> 

I fear I may need to add a lot of transitional code, which also requires 
to be understood, to keep the program working, so this splitting seems 
to be an "economy of scale" thing.
Right now I am not sure how much of this transitional code would be 
actually needed. I will give it a try.