Re: [PATCH vkd3d 04/12] vkd3d-shader/hlsl: Introduce add_load_component() in hlsl.y.

[Francisco Casas]

Hello,

On 05-07-22 06:26, Giovanni Mascellani wrote:
> Hi,
>
> about the patch subject, there is probably no real need to explicitly 
> mention hlsl.y.

Right, I will change that.
> Il 01/07/22 23:24, Francisco Casas ha scritto:
> > +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)
> > +{
> > +    struct hlsl_type *comp_type;
> > +    struct hlsl_ir_node *offset;
> > +    struct hlsl_ir_constant *c;
> > +    struct hlsl_ir_load *load;
> > +    unsigned int comp_offset;
> > +    struct hlsl_ir_var *var;
> > -
> >
> > +    comp_offset = hlsl_compute_component_offset(ctx, 
> > var_node->data_type, comp, &comp_type);
> > -
> >
> > +    if (!(c = hlsl_new_uint_constant(ctx, comp_offset, &loc)))
> > +        return NULL;
> > +    list_add_tail(instrs, &c->node.entry);
> > -
> >
> > +    offset = &c->node;
> > -
> >
> > +    if (var_node->type == HLSL_IR_LOAD)
> > +    {
> > +        const struct hlsl_deref *src = &hlsl_ir_load(var_node)->src;
> > +        struct hlsl_ir_node *add;
> > -
> >
> > +        var = src->var;
> > +        if (src->offset.node)
> > +        {
> > +            if (!(add = hlsl_new_binary_expr(ctx, HLSL_OP2_ADD, 
> > src->offset.node, &c->node)))
> > +                return NULL;
> > +            list_add_tail(instrs, &add->entry);
> > +            offset = add;
> > +        }
> > -
> >
> > +        if (!(load = hlsl_new_load(ctx, var, offset, comp_type, loc)))
> > +            return NULL;
> > +        list_add_tail(instrs, &load->node.entry);
> > +    }
> > +    else
> > +    {
> > +        struct hlsl_ir_store *store;
> > +        struct hlsl_ir_var *var;
> > +        char name[27];
> > -
> >
> > +        sprintf(name, "<deref-%p>", var_node);
>
> I think the new trend is to use string buffers. See how this is done in 
> intrinsic_mul(), for example.

I see. I will use string buffers in the next version.
> (if you ask me, I would also stop using global static counters, but 
> that's not happening yet; also, it's just a cosmetic issue)
> > +        if (!(var = hlsl_new_synthetic_var(ctx, name, 
> > var_node->data_type, var_node->loc)))
> > +            return NULL;
> > -
> >
> > +        if (!(store = hlsl_new_simple_store(ctx, var, var_node)))
> > +            return NULL;
> > +        list_add_tail(instrs, &store->node.entry);
> > -
> >
> > +        if (!(load = hlsl_new_load(ctx, var, offset, comp_type, loc)))
> > +            return NULL;
> > +        list_add_tail(instrs, &load->node.entry);
>
> This last block is identical in both branches, and it remains mostly 
> identical also in later patches. Maybe it could be brought outside of 
> the if, like it is in add_load()?

Good observation. I changed this in add_load_component() and 
add_load_index() for the next version. It requires moving some 
declarations to the outermost scope though.

I realized that here we have two "struct hlsl_ir_var *var;" declarations 
in different scopes too. I got rid of that too.
> Thanks, Giovanni.