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

[Zebediah Figura]

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.

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