From: Zebediah Figura Subject: Re: [PATCH 4/5] d3dcompiler: Don't use assignment instructions as sources. Message-Id: <872dd081-aa07-d128-38da-f3fce2d1352a@codeweavers.com> Date: Thu, 2 Jul 2020 11:58:10 -0500 In-Reply-To: References: <20200629235539.392976-1-zfigura@codeweavers.com> <20200629235539.392976-4-zfigura@codeweavers.com> On 7/2/20 11:15 AM, Matteo Bruni wrote: > On Tue, Jun 30, 2020 at 2:03 AM Zebediah Figura wrote: >> >> Signed-off-by: Zebediah Figura >> --- >> 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... > 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. > > If the temporary var + extra load solution doesn't fly, I guess this > could be the rare case where the unary + comes in handy, instead of > making up a new operator. > -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEENTo75Twe9rbPART3DZ01igeheEAFAl7+EiIACgkQDZ01igeh eECEgQf/XvMh1ZSBq2RmV/KqPJjAl/ZJuemDUKQvlPsoSSFZXZ5lbpS0MVFLWTOQ W6ff3KVUH2XkjWs/lj4u6UNA6L4tDtndyriozXLfKDfCXBqKFVTdK6qRU2kY7pQF /XIpPOZsDktLdIkIOHywQriqEp95aqWEiVCKAWlWBcbNPOmcs3FZS7rhaEArgosF o0qXsmzE+kIKI4KoANBlruZc2bR/x4qF4aBwWg5RH2TZSdhtCZfJWcz0hsUhp40p jsr7jFHMktt8uz00kb/pDAMy+1QSAj7Gj915tcSvbv+ulcPFeso3ugUEnaNgcCZ3 RXGE+v79pQsYDTe5BIWXrPBHMdo48w== =9F3h -----END PGP SIGNATURE-----