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

[Matteo Bruni]

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.