Re: [PATCH vkd3d 06/17] vkd3d-shader/hlsl: Introduce add_load_index().

[Matteo Bruni]

On Fri, Jul 15, 2022 at 3:24 AM Francisco Casas  wrote:
> At this point add_load() is split into add_load_component() and
> add_load_index(); register offsets are hidden for these functions.
>
> Signed-off-by: Francisco Casas

Nice patch. I guess this one is already pretty compelling evidence in
support of this "index paths" idea for parse-time IR.
> v2:
> - Use "const struct vkd3d_shader_location *loc" instead of "const struct
>
> vkd3d_shader_location loc"
> - Use vkd3d_string_buffer for initializing the deref synthetic variable names.
> - Move common "load = hlsl_new_load" pattern out of the if..else branches.
> - Removed braces on simple switch cases in
> hlsl_new_offset_from_path_index(). I kept them in the
> HLSL_CLASS_MATRIX case, even though it doesn't have declarations,
> because its body seems complex enough.
> Signed-off-by: Francisco Casas
>
> libs/vkd3d-shader/hlsl.c | 105 +++++++++++++++++++++++++++++-
>  libs/vkd3d-shader/hlsl.h |   7 ++
>  libs/vkd3d-shader/hlsl.y | 134 ++++++++++++++-------------------------
>  3 files changed, 157 insertions(+), 89 deletions(-)
>
> diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
> index 2925b4e5..e4e2ca17 100644
> --- a/libs/vkd3d-shader/hlsl.c
> +++ b/libs/vkd3d-shader/hlsl.c
> @@ -117,7 +117,7 @@ void hlsl_free_var(struct hlsl_ir_var *decl)
>      vkd3d_free(decl);
>  }
>
> -static bool hlsl_type_is_row_major(const struct hlsl_type *type)
> +bool hlsl_type_is_row_major(const struct hlsl_type *type)
>  {
>      /* Default to column-major if the majority isn't explicitly set, which can
>       * happen for anonymous nodes. */
> @@ -314,6 +314,109 @@ unsigned int hlsl_compute_component_offset(struct hlsl_ctx *ctx, struct hlsl_typ
>      return 0;
>  }
>
> +struct hlsl_type *hlsl_get_type_from_path_index(struct hlsl_ctx *ctx, const struct hlsl_type *type,
> -
>    struct hlsl_ir_node *node)

Maybe index, or field, or field_index? In the next function you have
"idx", which also works.
In any case, pick one and then use consistent naming throughout.

Also, I'm a bit confused by the function name: as far as I can see the
function has nothing specific to path indices, it's more of a "tell me
the type of the next inner component".
> +{
> - assert(node);
> -
> - if (type->type == HLSL_CLASS_VECTOR)
> -
>    return hlsl_get_scalar_type(ctx, type->base_type);
> -
> - if (type->type == HLSL_CLASS_MATRIX)
> - {
> -
>    if (hlsl_type_is_row_major(type))
> -
>        return hlsl_get_vector_type(ctx, type->base_type, type->dimx);
> -
>    else
> -
>        return hlsl_get_vector_type(ctx, type->base_type, type->dimy);
> - }
> -
> - if (type->type == HLSL_CLASS_ARRAY)
> -
>    return type->e.array.type;
> -
> - if (type->type == HLSL_CLASS_STRUCT)
> - {
> -
>    struct hlsl_ir_constant *c = hlsl_ir_constant(node);
> -
> -
>    assert(c->value[0].u < type->e.record.field_count);
> -
>    return type->e.record.fields[c->value[0].u].type;
> - }
> -
> - assert(0);
> - return NULL;
>
> +}
> -
>
> +struct hlsl_ir_node *hlsl_new_offset_from_path_index(struct hlsl_ctx *ctx, struct hlsl_block *block,
> -
>    struct hlsl_type *type, struct hlsl_ir_node *offset, struct hlsl_ir_node *idx,
> -
>    const struct vkd3d_shader_location *loc)
>
> +{
> - struct hlsl_ir_node *idx_offset = NULL;
> - struct hlsl_ir_constant *c;
> -
> - list_init(&block->instrs);
> -
> - switch (type->type)
> - {
> -
>    case HLSL_CLASS_VECTOR:
> -
>        idx_offset = idx;
> -
>        break;
> -
> -
>    case HLSL_CLASS_MATRIX:
> -
>    {
> -
>        if (!(c = hlsl_new_uint_constant(ctx, 4, loc)))
> -
>            return NULL;
> -
>        list_add_tail(&block->instrs, &c->node.entry);
> -
> -
>        if (!(idx_offset = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, &c->node, idx)))
> -
>            return NULL;
> -
>        list_add_tail(&block->instrs, &idx_offset->entry);
> -
> -
>        break;
> -
>    }
> -
> -
>    case HLSL_CLASS_ARRAY:
> -
>    {
> -
>        unsigned int size = hlsl_type_get_array_element_reg_size(type->e.array.type);
> -
> -
>        if (!(c = hlsl_new_uint_constant(ctx, size, loc)))
> -
>            return NULL;
> -
>        list_add_tail(&block->instrs, &c->node.entry);
> -
> -
>        if (!(idx_offset = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, &c->node, idx)))
> -
>            return NULL;
> -
>        list_add_tail(&block->instrs, &idx_offset->entry);
> -
> -
>        break;
> -
>    }
> -
> -
>    case HLSL_CLASS_STRUCT:
> -
>    {
> -
>        unsigned int field_i = hlsl_ir_constant(idx)->value[0].u;

field_idx?