[PATCH 4/5] d3dcompiler: Store derefs as an offset to a variable.

[Matteo Bruni]

On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12 at gmail.com> wrote:
>
> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
> ---
> One potential change to this patch is to always store an offset. Currently NULL
> is used to signify an offset of 0, partly to save space, and partly because
> otherwise new_var_deref() would need a bit of restructuring. But always using an
> offset would simplify some code.

Yeah, that might be a nice followup. Or, it feels like one, without
having actually touched the code at all...

>
>  dlls/d3dcompiler_43/d3dcompiler_private.h |  25 +---
>  dlls/d3dcompiler_43/hlsl.y                | 143 +++++++++++++---------
>  dlls/d3dcompiler_43/utils.c               |  76 ++----------
>  3 files changed, 97 insertions(+), 147 deletions(-)
>
> diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> index 387e932a6cc..983057d543c 100644
> --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> @@ -841,30 +841,11 @@ struct hlsl_ir_swizzle
>      DWORD swizzle;
>  };
>
> -enum hlsl_ir_deref_type
> -{
> -    HLSL_IR_DEREF_VAR,
> -    HLSL_IR_DEREF_ARRAY,
> -    HLSL_IR_DEREF_RECORD,
> -};
> -
>  struct hlsl_deref
>  {
> -    enum hlsl_ir_deref_type type;
> -    union
> -    {
> -        struct hlsl_ir_var *var;
> -        struct
> -        {
> -            struct hlsl_ir_node *array;
> -            struct hlsl_ir_node *index;
> -        } array;
> -        struct
> -        {
> -            struct hlsl_ir_node *record;
> -            struct hlsl_struct_field *field;
> -        } record;
> -    } v;
> +    struct hlsl_ir_var *var;
> +    struct hlsl_ir_node *offset;
> +    DWORD writemask;

I don't like moving the writemask into struct hlsl_deref. It's
something that makes sense only for an lvalue / assignment / STORE.
There are a few options here. Maybe turn struct hlsl_deref into struct
hlsl_lvalue and use it only in struct hlsl_ir_assignment (i.e. folding
the other use into struct hlsl_ir_deref while getting rid of the
writemask)? Do we lose a lot by "splitting" struct hlsl_deref?

>  };
>
>  struct hlsl_ir_deref
> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> index cc868757a26..24157c8fdc9 100644
> --- a/dlls/d3dcompiler_43/hlsl.y
> +++ b/dlls/d3dcompiler_43/hlsl.y
> @@ -538,15 +538,29 @@ static struct hlsl_ir_assignment *make_simple_assignment(struct hlsl_ir_var *lhs
>          return NULL;
>
>      init_node(&assign->node, HLSL_IR_ASSIGNMENT, rhs->data_type, rhs->loc);
> -    assign->lhs.type = HLSL_IR_DEREF_VAR;
> -    assign->lhs.v.var = lhs;
> +    assign->lhs.var = lhs;
>      assign->rhs = rhs;
>      if (type_is_single_reg(lhs->data_type))
> -        assign->writemask = (1 << lhs->data_type->dimx) - 1;
> +        assign->lhs.writemask = assign->writemask = (1 << lhs->data_type->dimx) - 1;
>
>      return assign;
>  }
>
> +static struct hlsl_ir_constant *new_uint_constant(unsigned int n, const struct source_location loc)
> +{
> +    struct hlsl_type *type;
> +    struct hlsl_ir_constant *c;
> +
> +    if (!(type = new_hlsl_type(d3dcompiler_strdup("uint"), HLSL_CLASS_SCALAR, HLSL_TYPE_UINT, 1, 1)))
> +        return NULL;

En passant: I was thinking that we could do something to avoid
redundantly creating hlsl_types over and over, at least for the
"basic" types. Probably something very simple, like storing each
scalar type in an array, would cover a lot already.

> +
> +    if (!(c = d3dcompiler_alloc(sizeof(*c))))
> +        return NULL;
> +    init_node(&c->node, HLSL_IR_CONSTANT, type, loc);
> +    c->v.value.u[0] = n;
> +    return c;
> +}

WRT memory usage, in particular when always storing the offset: we
could allocate only the required portion of the whole struct
hlsl_ir_constant i.e. something along the lines of
"d3dcompiler_alloc(FIELD_OFFSET(struct hlsl_ir_constant,
v.value.u[1]))". If the "tricky" allocation happens in a single place,
like a new_constant() helper or some-such, maybe it won't be too
ugly...
BTW I'm not sure that the rest of the compiler is ready for this kind
of thing. Let's call it another "nice to have at some point".

> +
>  static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct source_location loc)
>  {
>      struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
> @@ -557,60 +571,76 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
>          return NULL;
>      }
>      init_node(&deref->node, HLSL_IR_DEREF, var->data_type, loc);
> -    deref->src.type = HLSL_IR_DEREF_VAR;
> -    deref->src.v.var = var;
> +    deref->src.var = var;
> +    if (type_is_single_reg(var->data_type))
> +        deref->src.writemask = (1 << var->data_type->dimx) - 1;
>      return deref;
>  }
>
> -static struct hlsl_ir_node *get_var_deref(struct hlsl_ir_node *node)
> +static struct hlsl_ir_deref *new_deref(struct hlsl_ir_node *var_node, struct hlsl_ir_node *offset,
> +        struct hlsl_type *data_type, const struct source_location loc)
>  {
> -    struct hlsl_ir_assignment *assign;
> +    struct hlsl_ir_node *add = NULL;
>      struct hlsl_ir_deref *deref;
>      struct hlsl_ir_var *var;
> -    char name[27];
>
> -    if (node->type == HLSL_IR_DEREF)
> -        return node;
> +    if (var_node->type == HLSL_IR_DEREF)
> +    {
> +        const struct hlsl_deref *src = &deref_from_node(var_node)->src;
>
> -    sprintf(name, "<deref-%p>", node);
> -    if (!(var = new_synthetic_var(name, node->data_type, node->loc)))
> -        return NULL;
> +        var = src->var;
> +        if (src->offset)
> +        {
> +            if (!(add = new_binary_expr(HLSL_IR_BINOP_ADD, src->offset, offset, loc)))
> +                return NULL;
> +            list_add_after(&offset->entry, &add->entry);
> +            offset = add;
> +        }
> +    }
> +    else
> +    {
> +        struct hlsl_ir_assignment *assign;
> +        char name[27];
> +
> +        sprintf(name, "<deref-%p>", var_node);
> +        if (!(var = new_synthetic_var(name, var_node->data_type, var_node->loc)))
> +            return NULL;
>
> -    TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(node->type));
> +        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(var_node->type));
>
> -    if (!(assign = make_simple_assignment(var, node)))
> -        return NULL;
> -    list_add_after(&node->entry, &assign->node.entry);
> +        if (!(assign = make_simple_assignment(var, var_node)))
> +            return NULL;
>
> -    if (!(deref = new_var_deref(var, var->loc)))
> +        list_add_after(&var_node->entry, &assign->node.entry);
> +    }
> +
> +    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
>          return NULL;
> -    list_add_after(&assign->node.entry, &deref->node.entry);
> -    return &deref->node;
> +    init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
> +    deref->src.var = var;
> +    deref->src.offset = offset;
> +    if (type_is_single_reg(data_type))
> +        deref->src.writemask = (1 << data_type->dimx) - 1;
> +    list_add_after(&offset->entry, &deref->node.entry);
> +    return deref;
>  }

How bad is it to avoid creating "extra" deref nodes? Mostly curious,
it's okay to leave dead code around.