[PATCH 4/5] d3dcompiler: Don't use assignment instructions as sources.

[Matteo Bruni]

On Thu, Jul 2, 2020 at 6:58 PM Zebediah Figura <zfigura at codeweavers.com> wrote:
>
> On 7/2/20 11:15 AM, Matteo Bruni wrote:
> > On Tue, Jun 30, 2020 at 2:03 AM Zebediah Figura <z.figura12 at gmail.com> wrote:
> >>
> >> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
> >> ---
> >>  dlls/d3dcompiler_43/d3dcompiler_private.h |  2 ++
> >>  dlls/d3dcompiler_43/utils.c               | 14 ++++++++++++--
> >>  2 files changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> >> index 23eff210940..2ff019d45a6 100644
> >> --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> >> +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> >> @@ -786,6 +786,8 @@ enum hlsl_ir_expr_op {
> >>      HLSL_IR_UNOP_POSTINC,
> >>      HLSL_IR_UNOP_POSTDEC,
> >>
> >> +    HLSL_IR_UNOP_IDENT,
> >> +
> >>      HLSL_IR_BINOP_ADD,
> >>      HLSL_IR_BINOP_SUB,
> >>      HLSL_IR_BINOP_MUL,
> >> diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
> >> index 4bc3b9b0a64..b5422abd1e8 100644
> >> --- a/dlls/d3dcompiler_43/utils.c
> >> +++ b/dlls/d3dcompiler_43/utils.c
> >> @@ -1448,6 +1448,7 @@ struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lh
> >>  {
> >>      struct hlsl_ir_assignment *assign = d3dcompiler_alloc(sizeof(*assign));
> >>      struct hlsl_type *lhs_type;
> >> +    struct hlsl_ir_node *copy;
> >>      DWORD writemask = 0;
> >>
> >>      lhs_type = lhs->data_type;
> >> @@ -1511,7 +1512,7 @@ struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lh
> >>          lhs = lhs_inner;
> >>      }
> >>
> >> -    init_node(&assign->node, HLSL_IR_ASSIGNMENT, lhs_type, lhs->loc);
> >> +    init_node(&assign->node, HLSL_IR_ASSIGNMENT, NULL, lhs->loc);
> >>      assign->writemask = writemask;
> >>      assign->lhs.var = load_from_node(lhs)->src.var;
> >>      hlsl_src_from_node(&assign->lhs.offset, load_from_node(lhs)->src.offset.node);
> >> @@ -1528,7 +1529,14 @@ struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lh
> >>      hlsl_src_from_node(&assign->rhs, rhs);
> >>      list_add_tail(instrs, &assign->node.entry);
> >>
> >> -    return &assign->node;
> >> +    /* Don't use the instruction itself as a source, as this makes structure
> >> +     * splitting easier. Instead copy it here. Since we retrieve sources from
> >> +     * the last instruction in the list, we do need to copy. */
> >
> > Not just that, what we now define as an assignment (really a STORE)
> > doesn't really make sense anymore as a source.
> >
> > The need to copy the instruction is a bit annoying though. What about
> > storing the result into a temporary variable and adding an extra load
> > instead?
>
> It didn't seem obviously better to me, given the extra allocation, but I
> think there's no reason it can't work. Certainly it avoids 5/5...

Yeah, my feeling is that these new loads won't be different from other
redundant loads that come up in shaders during compilation (i.e. copy
propagation of some kind can scoop up all of them).

> > Does it make struct splitting hard / impossible (e.g. along
> > the lines of: replace this instruction with this other instruction and
> > the copy -> now we need to replace the copy -> back to square one)?
>
> The algorithm I have basically generates store + load instructions for
> each struct field, then removes the original. This breaks if the
> original is used as a source, so I figured it was best if we just didn't
> do that.
>
> The other easy solution I see is to iterate through the instruction list
> in reverse.

Right. I'm convinced that never using an assignment / store as a
source is just a good idea, regardless.