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

[Matteo Bruni]

On Wed, Aug 3, 2022 at 11:26 PM Francisco Casas <fcasas at codeweavers.com> wrote:
>
>
>
> 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".

"Instr" should always work as a replacement. Node really isn't an
appropriate name anymore, specifically since we flattened the IR a
while back.