[PATCH 4/5] d3dcompiler: Store derefs as an offset to a variable.
Zebediah Figura
zfigura at codeweavers.com
Tue May 19 15:10:26 CDT 2020
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.
>
>> };
>>
>> 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.
Yes, that's a good idea.
>
>> +
>> + 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...
>
>> +
>> 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.)
More information about the wine-devel
mailing list