[PATCH 4/5] d3dcompiler: Store derefs as an offset to a variable.
Matteo Bruni
matteo.mystral at gmail.com
Tue May 19 17:07:13 CDT 2020
On Tue, May 19, 2020 at 11:14 PM Zebediah Figura
<zfigura at codeweavers.com> wrote:
>
> On 5/19/20 3:44 PM, Matteo Bruni wrote:
> > 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...
>
> Yes, you're right. I've been dealing with the offset in registers, but
> there's not really a good reason to do that...
>
> >
> >>>> +
> >>>> + 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.
>
> I think that in any such case we also have to replace the new
> instruction, though. Put another way, if an instruction consumes a
> matrix, then it still needs to consume a matrix even once the
> instruction producing a matrix has been split to produce vectors. I
> don't think it ever makes sense to change the source of an instruction
> to be a different type.
>
> To try to express how I'm planning to split things, given an expression
> like "mat1 = mat2 * 2" we generate
>
> 2: load(mat2)
> 3: 2
> 4: @2 * @3
> 5: mat1 = @4
>
> We split @4, yielding
>
> 2: load(mat2)
> 3: 2
> 4: @2 * @3
> 5: load(mat2[0])
> 6: @5 * @3
> 7: <syn-4>[0] = @6
> 8: load(mat2[1])
> 9: @8 * @3
> 10: <syn-4>[1] = @9
> 11: mat1 = @4
>
> We split @11, yielding
>
> ...
> 10: <syn-4>[1] = @9
> 11: mat1 = @4
> 12: load(<syn-4>[0])
> 13: mat1[0] = @12
> 14: load(<syn-4>[1])
> 15: mat1[1] = @14
>
> @11 is removed after splitting, and @4 is subsequently dce'd out. The
> crucially relevant bit is that we keep both instructions around after
> splitting them, and we don't actually change the source. This was the
> simplest approach I could come up with, because it means we can
> essentially split one instruction at a time, without having to recurse.
Cool. Yeah, that looks simpler than what I was envisioning. I'm not
sure how turning struct hlsl_ir_node into a union would help, or
matter, though.
More information about the wine-devel
mailing list