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

[Giovanni Mascellani]

Hi,

about the patch subject, there is probably no real need to explicitly 
mention hlsl.y.

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.

(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()?

Thanks, Giovanni.