[PATCH vkd3d 05/17] vkd3d-shader/hlsl: Introduce add_load_component().

[Francisco Casas]

On 28-07-22 16:44, Matteo Bruni wrote:
> Okay, I guess I managed to read the patch series through. Sorry it
> took me way longer than I'd hoped, and I didn't even properly review
> the individual patches (e.g. apply them and check that the tests pass
> after each one).
> 
> In general the patch series seems okay to me and, as far as I'm
> concerned, if this goes in with no changes at all I would have no
> qualms.
> 
> That said, I do have a number of relatively minor comments that I'll
> send in reply to individual patches. For the earlier patches in the
> series (like this one) I'm going to reply to "v2" since that's where I
> had started to write them, but I don't think anything mentioned in
> those was affected by the changes in v3.
> 
> On Fri, Jul 15, 2022 at 3:24 AM Francisco Casas <fcasas at codeweavers.com> wrote:
>>
>> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
>> ---
>>
>> v2:
>> * Use "const struct vkd3d_shader_location *" instead of "const struct
>>    vkd3d_shader_location".
>> * Removed 'in hlsl.y' in the patch subject.
>> * Use vkd3d_string_buffer for initializing the deref synthetic variable names.
>> * Move common "load = hlsl_new_load" pattern out of the if..else branches.
>>
>> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
>> ---
>>   libs/vkd3d-shader/hlsl.y | 98 +++++++++++++++++++++++++++-------------
>>   1 file changed, 67 insertions(+), 31 deletions(-)
>>
>> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
>> index bfafd981..f5c9f4ca 100644
>> --- a/libs/vkd3d-shader/hlsl.y
>> +++ b/libs/vkd3d-shader/hlsl.y
>> @@ -272,8 +272,8 @@ static bool implicit_compatible_data_types(struct hlsl_type *t1, struct hlsl_typ
>>       return false;
>>   }
>>
>> -static struct hlsl_ir_load *add_load(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *var_node,
>> -        struct hlsl_ir_node *offset, struct hlsl_type *data_type, const struct vkd3d_shader_location loc);
>> +static struct hlsl_ir_load *add_load_component(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *var_node,
>> +        unsigned int comp, const struct vkd3d_shader_location *loc);
>>
>>   static struct hlsl_ir_node *add_cast(struct hlsl_ctx *ctx, struct list *instrs,
>>           struct hlsl_ir_node *node, struct hlsl_type *dst_type, const struct vkd3d_shader_location *loc)
>> @@ -311,8 +311,8 @@ static struct hlsl_ir_node *add_cast(struct hlsl_ctx *ctx, struct list *instrs,
>>
>>           for (dst_idx = 0; dst_idx < dst_type->dimx * dst_type->dimy; ++dst_idx)
>>           {
>> -            struct hlsl_type *src_scalar_type, *dst_scalar_type;
>> -            unsigned int src_idx, src_offset, dst_offset;
>> +            struct hlsl_type *dst_scalar_type;
>> +            unsigned int src_idx, dst_offset;
>>               struct hlsl_ir_store *store;
>>               struct hlsl_ir_constant *c;
>>
>> @@ -335,13 +335,8 @@ 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);
>> -            src_offset = hlsl_compute_component_offset(ctx, src_type, src_idx, &src_scalar_type);
>> -
>> -            if (!(c = hlsl_new_uint_constant(ctx, src_offset, loc)))
>> -                return NULL;
>> -            list_add_tail(instrs, &c->node.entry);
>>
>> -            if (!(load = add_load(ctx, instrs, node, &c->node, src_scalar_type, *loc)))
>> +            if (!(load = add_load_component(ctx, instrs, node, src_idx, loc)))
>>                   return NULL;
>>
>>               if (!(cast = hlsl_new_cast(ctx, &load->node, dst_scalar_type, loc)))
>> @@ -668,6 +663,62 @@ static struct hlsl_ir_load *add_load(struct hlsl_ctx *ctx, struct list *instrs,
>>       return load;
>>   }
>>
>> +static struct hlsl_ir_load *add_load_component(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *var_node,
>> +        unsigned int comp, const struct vkd3d_shader_location *loc)
> 
> I would avoid using any "node" variable name in new code.

Understood. However, in this particular case (and later, 
add_load_index()) I can't think of a better name for the node argument, 
besides "value".