[PATCH 7/7] d3dcompiler: Store the "array" field in struct hlsl_deref as a struct hlsl_deref pointer.

[Matteo Bruni]

On Thu, Mar 12, 2020 at 4:12 PM Zebediah Figura <zfigura at codeweavers.com> wrote:
>
> On 3/12/20 6:58 AM, Matteo Bruni wrote:
> > On Wed, Mar 11, 2020 at 8:45 PM Zebediah Figura <zfigura at codeweavers.com> wrote:
> >>
> >> On 3/11/20 12:57 PM, Matteo Bruni wrote:
> >>> On Sat, Mar 7, 2020 at 12:25 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/hlsl.y                | 25 ++++++++++++++++++++---
> >>>>   dlls/d3dcompiler_43/utils.c               |  6 +++++-
> >>>>   3 files changed, 28 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> >>>> index d221d1056fa..f4f68b9e4cf 100644
> >>>> --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> >>>> +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> >>>> @@ -907,7 +907,7 @@ struct hlsl_deref
> >>>>           struct hlsl_ir_var *var;
> >>>>           struct
> >>>>           {
> >>>> -            struct hlsl_ir_node *array;
> >>>> +            struct hlsl_deref *array;
> >>>>               struct hlsl_ir_node *index;
> >>>>           } array;
> >>>>           struct
> >>>> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> >>>> index 40159c60dcc..11dd8026c16 100644
> >>>> --- a/dlls/d3dcompiler_43/hlsl.y
> >>>> +++ b/dlls/d3dcompiler_43/hlsl.y
> >>>> @@ -2114,8 +2114,17 @@ postfix_expr:             primary_expr
> >>>>                                   /* This may be an array dereference or a vector/matrix
> >>>>                                    * subcomponent access.
> >>>>                                    * We store it as an array dereference in any case. */
> >>>> -                                struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
> >>>> -                                struct hlsl_type *expr_type = node_from_list($1)->data_type;
> >>>> +                                struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref)), *src;
> >>>> +                                struct hlsl_ir_node *node = node_from_list($1);
> >>>> +                                struct hlsl_type *expr_type = node->data_type;
> >>>> +
> >>>> +                                if (node->type != HLSL_IR_DEREF)
> >>>> +                                {
> >>>> +                                    FIXME("Unimplemented index of node type %s.\n",
> >>>> +                                            debug_node_type(node->type));
> >>>> +                                    YYABORT;
> >>>> +                                }
> >>>> +                                src = deref_from_node(node);
> >>>
> >>> It looks like it's legal in HLSL to array dereference an arbitrary
> >>> vector expression. E.g.:
> >>>
> >>> uniform int i;
> >>> uniform float4 b;
> >>> ...
> >>> float4 a = {0.25, 0.5, 0.75, 1.0};
> >>> float r = (a * b)[i];
> >>>
> >>> How do you plan to handle that? Mostly wondering if this patch would
> >>> have to be effectively reverted in the future or you have something
> >>> else in mind.
> >>>
> >>
> >> Honestly, I was aware of this, and my plan was basically to ignore it
> >> and deal with it when we come to it, if we ever do.
> >>
> >> I don't remember if there's a similar problem with struct fields, but
> >> there probably is. If nothing else I would expect
> >> "func_that_returns_struct().field" to work.
> >>
> >> The main reason I want this patch is because of the LHS. It only makes
> >> sense to assign to a variable, valid HLSL expressions like "(++a)[0] =
> >> b" notwithstanding. That gives us a clear separation between "variables"
> >> and "anonymous expressions" which makes liveness tracking easier or at
> >> least clearer (and also lets anonymous expressions be SSA values—not a
> >> design choice I've taken advantage of thus far, but if in the future we
> >> need to start doing optimizations, it's nice to have the compiler in a
> >> state that makes that easy.)
> >>
> >> Currently my plan is to track liveness per variable (instead of e.g. per
> >> element or field), so I have a function hlsl_var_from_deref() which
> >> unwraps the hlsl_deref until it gets the initial variable. If it helps I
> >> can post those patches here.
> >
> > It's not necessary, I understand your requirements[*]. I did basically
> > the same thing in my hacky compiler. I'll mention that this is, in
> > some way, one of the downsides of using the same IR for most / all of
> > the compilation process. But it's not a blocker by any means.
> >
> > It's possible to solve the problem in a number of ways. Leaving aside
> > full-blown SSA for the time being, we can still introduce shader
> > transformation passes that can help in this regard. Probably the
> > easiest would be a pass to split complex expressions into separate
> > instructions, each in the form of a "conformant" assignment (i.e. to a
> > temporary variable if necessary). So for my example above, you'd get
> > something like:
> >
> >>> float r = (a * b)[i];
> >
> > 1: deref(a)
> > 2: deref(b)
> > 3: * (a b)
> > 4: deref(temp1)
> > 5: = (4 3)
> > 6: deref(i)
> > 7: deref(temp1)[6]
> > 8: deref(r)
> > 9: = (8 7)
>
> Ah, so basically synthesize a variable every time we try to
> index/subscript something that's not already a deref? Seems like an
> attractive idea, and in fact would even work nicely with the example I
> mentioned above ["(++a)[0] = b" apparently has no effect.]
>
> Though admittedly I am warming up to my last suggestion, which kind of
> renders this unnecessary.
>
> >
> > (I certainly didn't pick the nicest convention for those IR dumps, oh well...)
> >
> > Doing this kind of transformation before the liveness analysis should
> > solve the issue at hand (and I'm pretty sure it would help with other
> > optimization / codegen things).
> > This calls for the question: how, and how much, should the IR before /
> > after the pass be segregated? E.g. we could use the current
> > hlsl_ir_deref and hlsl_ir_assignment only for the "before" and
> > introduce separate versions for the "after" which enforce the "only
> > derefs to variables" invariant. Other options would be to always use
> > the current IR types, probably with some additional checks to ensure
> > things are sane, or at the opposite end, create a whole separate,
> > lower level IR (which could differ from the current IR in more ways
> > than just the derefs, depending on what else might come in handy).
> > Relatedly: I think it would be useful to have some form of validation
> > (probably only enabled for warn+d3dcompiler or similar) to check that
> > the shader IR invariants are respected at various points of the
> > compilation process.
>
> Eh, maybe. I haven't found the language restrictive enough thus far to
> warrant any new layers. Probably that'd change if I had to do serious
> optimization, but thus far...

Sure, no reason to add more machinery until necessary.