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

Matteo Bruni matteo.mystral at gmail.com
Tue May 19 15:44:49 CDT 2020


On Tue, May 19, 2020 at 10:11 PM Zebediah Figura
<zfigura at codeweavers.com> wrote:
>
> On 5/19/20 2:34 PM, Matteo Bruni wrote:
> > 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?
>
> "writemask" may be a bit of a misnomer, since there's not necessarily
> writing (maybe "mask" is better?), but it's relevant for loads too. It's
> essentially a companion to 'offset', to describe which part of the vec4
> should be read from or written to. For loads we convert the mask into a
> swizzle.

It shouldn't be necessary though, if the offset is in the unit of
(scalar) components rather than 4-component registers. Per-component
offset, together with the data type should be enough.
Now, of course I haven't really thought it through so there might be dragons...

> >> +
> >> +    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".
>
> This is potentially a good idea, but it also conflicts with one of two
> possible ideas I had for a different problem:
>
> I think we need some way of replacing instructions after parse time,
> including with instructions of a different type. It's useful for
> splitting, constant folding, lowering of one operation to another, and
> so on.
>
> One way to do this is to track def-use chains. This also lets us
> simplify liveness analysis a bit, but it's not an awful lot of code that
> we save that way.
>
> The other thing that we could do is to make hlsl_ir_node a union. I
> think this makes some code simpler, of course at the expense of using a
> bit more memory.
>
> Ultimately I don't have a strong preference, but it is something that'd
> be nice to work out soon, as it does strongly affect the structure of
> almost all code I subsequently write...

So your idea is to replace an instruction with a different one "in
place" so that all the pointers that refer to it don't need to be
updated. My gut feeling is that it might not be enough in the general
case. In particular, if you split an instruction into multiple
instructions (e.g. maybe some matrix operation into per-vector
arithmetic) you might want to point a users of the original
instruction to any of the new ones.
This kind of transformation would break "plain" def-use chain
replacement too, but that seems solvable.

> >> +
> >>  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.
> >
>
> So I'm guessing that what you mean by this is: given an expression like
> "a.b.c", instead of
>
> 2: a
> 3: 2 # offset of b
> 4: a[@3]
> 5: 2 # offset of c
> 6: @3 + @5
> 7: a[@6]
>
> we delete or replace @2 and @4 when generating @4 and @7, respectively.
>
> It's a bit tricky, because while in practice I think that instructions
> @2 and @4 will have no uses, that's not really spelled out as a
> guarantee anywhere, and having it be a thing to implicitly remember
> makes me a *little* nervous. (I'm not sure I trust myself to implicitly
> remember it.) If we track def-use chains, that's less of a problem, but
> I'm still not sure whether we want to track def-use chains—see above.
>
> (Arguably we need def-use anyway, for DCE, but we don't really—we only
> need to check whether a field is used, not where, so we can just reuse
> the liveness analysis passes we already have.)

Yes, that was my question, and yeah, I see. Definitely no point in
complicating this for no reason aside from making the IR nicer to
read.

Also agreed, the current liveness stuff should be enough for DCE,
especially for the time being.



More information about the wine-devel mailing list