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

[Zebediah Figura]

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

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