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

[Matteo Bruni]

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)

(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.

> As an alternative to 6/7 and 7/7 here, I could move the hlsl_node
> unwrapping to hlsl_var_from_deref() [i.e. so that function checks the
> node type of the "array" field and fails noisily for now if it's not
> HLSL_IR_DEREF]. I don't particularly like that, both because it kind of
> suggests that the internal representation of an LHS is allowed to not be
> reducible to a variable, which is not great, and because codegen time is
> not a particularly nice time to do those kinds of checks.
>
> A middle ground could be to do those checks only for the lhs in
> make_assignment() or whatever it's called, leave them alone otherwise,
> and then just throw a few assert()s in hlsl_var_from_deref().

I think this, combined with a transformation pass like the one I
suggested (once you get to liveness), is a good option: you get to
check for broken assignment LHS early and don't hamper support for
those kind of fancy, but still valid, language constructs.

[*]: Or at least I think I do; correct me if I'm off.