[PATCH 2/5] d3dcompiler: Introduce a separate structure for lvalue derefs.
Matteo Bruni
matteo.mystral at gmail.com
Fri Apr 17 02:03:45 CDT 2020
On Wed, Apr 15, 2020 at 2:41 AM Zebediah Figura <z.figura12 at gmail.com> wrote:
>
> From: Zebediah Figura <zfigura at codeweavers.com>
>
> Signed-off-by: Zebediah Figura <z.figura12 at gmail.com>
> ---
> aaa625217, and the approach it implies, works, but on further reflection it's
> still not very pretty. For example, the following line of HLSL:
>
> var.a.b = 2.0;
>
> produces the following IR:
>
> 2: 2.0
> 3: deref(var)
> 4: @3.b
> 5: @4.c = @2
>
> This essentially works, provided that the codegen layer knows how to unwind a
> deref chain, but it's pretty janky. Node 4 implies we're reading from node 3,
> and node 3 implies we're reading from var, neither of which is actually
> happening. The RA pass can't easily be smart enough to recognize that 4 doesn't
> need to read from 3, which is a problem if we introduce a "maybe uninitialized"
> warning.
>
> Moreover, if we introduce DCE, we can't DCE out node 3 because of node 4, and
> while we could DCE out node 4 in terms of removing it from the list, we can't
> actually destroy the node, since node 5 is using it. Having nodes not in the
> list is also janky, I feel.
>
> With this patch we instead get the following IR:
>
> 2: 2.0
> 3: deref(var)
> 4: @3.b
> 5: @4.c
> 6: deref(var).b.c = @2
>
> We still get redundant reads, but at least this time we can easily DCE them
> out. Needing less sanity checks in RA is also nice.
>
> dlls/d3dcompiler_43/d3dcompiler_private.h | 21 +++++-
> dlls/d3dcompiler_43/hlsl.y | 28 +++++---
> dlls/d3dcompiler_43/utils.c | 83 ++++++++++++++++-------
> 3 files changed, 97 insertions(+), 35 deletions(-)
Hi Zeb,
after reading it again and actually getting into "review mode", I have
some more thoughts... Sorry for not thinking of this earlier.
See below.
> diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> index 1f1d8e26637..8f81271383b 100644
> --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> @@ -873,10 +873,29 @@ struct hlsl_ir_deref
> struct hlsl_deref src;
> };
>
> +struct hlsl_lvalue
> +{
> + enum hlsl_ir_deref_type type;
> + union
> + {
> + struct hlsl_ir_var *var;
> + struct
> + {
> + struct hlsl_lvalue *array;
> + struct hlsl_ir_node *index;
> + } array;
> + struct
> + {
> + struct hlsl_lvalue *record;
> + struct hlsl_struct_field *field;
> + } record;
> + } v;
> +};
> +
> struct hlsl_ir_assignment
> {
> struct hlsl_ir_node node;
> - struct hlsl_deref lhs;
> + struct hlsl_lvalue lhs;
> struct hlsl_ir_node *rhs;
> unsigned char writemask;
> };
> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> index d3ef18bb6e9..5366181f356 100644
> --- a/dlls/d3dcompiler_43/hlsl.y
> +++ b/dlls/d3dcompiler_43/hlsl.y
> @@ -2642,16 +2642,16 @@ static unsigned int index_instructions(struct list *instrs, unsigned int index)
> }
>
> /* Walk the chain of derefs and retrieve the actual variable we care about. */
> -static struct hlsl_ir_var *hlsl_var_from_deref(const struct hlsl_deref *deref)
> +static struct hlsl_ir_var *hlsl_var_from_lvalue(const struct hlsl_lvalue *deref)
> {
> switch (deref->type)
> {
> case HLSL_IR_DEREF_VAR:
> return deref->v.var;
> case HLSL_IR_DEREF_ARRAY:
> - return hlsl_var_from_deref(&deref_from_node(deref->v.array.array)->src);
> + return hlsl_var_from_lvalue(deref->v.array.array);
> case HLSL_IR_DEREF_RECORD:
> - return hlsl_var_from_deref(&deref_from_node(deref->v.record.record)->src);
> + return hlsl_var_from_lvalue(deref->v.record.record);
> }
> assert(0);
> return NULL;
> @@ -2674,7 +2674,7 @@ static void compute_liveness_recurse(struct list *instrs, unsigned int loop_firs
> case HLSL_IR_ASSIGNMENT:
> {
> struct hlsl_ir_assignment *assignment = assignment_from_node(instr);
> - var = hlsl_var_from_deref(&assignment->lhs);
> + var = hlsl_var_from_lvalue(&assignment->lhs);
> if (!var->first_write)
> var->first_write = loop_first ? min(instr->index, loop_first) : instr->index;
> assignment->rhs->last_read = instr->index;
> @@ -2693,10 +2693,22 @@ static void compute_liveness_recurse(struct list *instrs, unsigned int loop_firs
> case HLSL_IR_DEREF:
> {
> struct hlsl_ir_deref *deref = deref_from_node(instr);
> - var = hlsl_var_from_deref(&deref->src);
> - var->last_read = loop_last ? max(instr->index, loop_last) : instr->index;
> - if (deref->src.type == HLSL_IR_DEREF_ARRAY)
> - deref->src.v.array.index->last_read = instr->index;
> +
> + switch (deref->src.type)
> + {
> + case HLSL_IR_DEREF_VAR:
> + deref->src.v.var->last_read = loop_last ? max(instr->index, loop_last) : instr->index;
> + break;
> +
> + case HLSL_IR_DEREF_ARRAY:
> + deref->src.v.array.array->last_read = instr->index;
> + deref->src.v.array.index->last_read = instr->index;
> + break;
> +
> + case HLSL_IR_DEREF_RECORD:
> + deref->src.v.record.record->last_read = instr->index;
> + break;
> + }
> break;
> }
> case HLSL_IR_EXPR:
> diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
> index d24341329d3..519f50612e8 100644
> --- a/dlls/d3dcompiler_43/utils.c
> +++ b/dlls/d3dcompiler_43/utils.c
> @@ -1478,27 +1478,41 @@ static unsigned int invert_swizzle(unsigned int *swizzle, unsigned int writemask
> return new_writemask;
> }
>
> -static BOOL validate_lhs_deref(const struct hlsl_ir_node *lhs)
> +static BOOL get_assignment_lhs(struct hlsl_lvalue *dst, struct hlsl_ir_node *node)
> {
> struct hlsl_ir_deref *deref;
>
> - if (lhs->type != HLSL_IR_DEREF)
> + if (node->type != HLSL_IR_DEREF)
> {
> - hlsl_report_message(lhs->loc, HLSL_LEVEL_ERROR, "invalid lvalue");
> + hlsl_report_message(node->loc, HLSL_LEVEL_ERROR, "invalid lvalue");
> return FALSE;
> }
>
> - deref = deref_from_node(lhs);
> + deref = deref_from_node(node);
> + dst->type = deref->src.type;
>
> - if (deref->src.type == HLSL_IR_DEREF_VAR)
> - return TRUE;
> - if (deref->src.type == HLSL_IR_DEREF_ARRAY)
> - return validate_lhs_deref(deref->src.v.array.array);
> - if (deref->src.type == HLSL_IR_DEREF_RECORD)
> - return validate_lhs_deref(deref->src.v.record.record);
> + switch (deref->src.type)
> + {
> + case HLSL_IR_DEREF_VAR:
> + dst->v.var = deref->src.v.var;
> + return TRUE;
>
> - assert(0);
> - return FALSE;
> + case HLSL_IR_DEREF_ARRAY:
> + dst->v.array.index = deref->src.v.array.index;
> + if (!(dst->v.array.array = d3dcompiler_alloc(sizeof(*dst->v.array.array))))
> + return FALSE;
> + return get_assignment_lhs(dst->v.array.array, deref->src.v.array.array);
> +
> + case HLSL_IR_DEREF_RECORD:
> + dst->v.record.field = deref->src.v.record.field;
> + if (!(dst->v.record.record = d3dcompiler_alloc(sizeof(*dst->v.record.field))))
> + return FALSE;
> + return get_assignment_lhs(dst->v.record.record, deref->src.v.record.record);
> +
> + default:
> + assert(0);
> + return FALSE;
> + }
> }
I'm somewhat annoyed by this, it feels like we could directly create
lvalue nodes while parsing... Except there doesn't seem to be any
simple way. So I'm going to try and keep my initial knee-jerk reaction
at bay.
On the other hand, I think it would be nicer to enforce a flat IR
structure. Instead of having an arbitrarily complex lvalue inside the
assignment instruction, maybe it should have the same structure of a
deref, where each "level" is a separate instruction. This is probably
why I was thinking of a flag rather than a separate node type: when
constructing the assignment you would traverse the lhs derefs and turn
them into lvalues. Although having a separate node type and changing
that works as well (and it's probably nicer with liveness computation
and other passes).
Does that sound reasonable? Am I missing anything?
> +static void debug_dump_lvalue(const struct hlsl_lvalue *deref)
> +{
> + switch (deref->type)
> + {
> + case HLSL_IR_DEREF_VAR:
> + debug_dump_ir_var(deref->v.var);
> + break;
> + case HLSL_IR_DEREF_ARRAY:
> + debug_dump_lvalue(deref->v.array.array);
> + wine_dbg_printf("[");
> + debug_dump_src(deref->v.array.index);
> + wine_dbg_printf("]");
> + break;
> + case HLSL_IR_DEREF_RECORD:
> + debug_dump_lvalue(deref->v.record.record);
> + wine_dbg_printf(".%s", debugstr_a(deref->v.record.field->name));
> + break;
> + }
> +}
If we go with my idea, we probably need something to disambiguate
lvalues from rvalues / plain derefs.
More information about the wine-devel
mailing list