[PATCH 7/7] d3dcompiler: Store the "array" field in struct hlsl_deref as a struct hlsl_deref pointer.
Zebediah Figura
zfigura at codeweavers.com
Mon Apr 6 20:52:18 CDT 2020
On 3/12/20 10:12 AM, Zebediah Figura 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...
>
>>
>>> 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.
>>
>
I gave it some examination and though and I decided I really don't like
the current solution, for reasons which I mostly go into detail about in
the attached patch 0001. I came up with two alternative approaches, and
I'm not really sure which one I prefer:
* patch 0001 just separates source and destination derefs, forcing the
latter to be a chain of derefs ending in a var while the former can end
in an arbitrary node, which has the advantage of generating less HLSL
(not that it particularly matters);
* patches 0100-0102 synthesizes a variable every time we encounter a
subscript/index of anything other than a variable, thus implicitly
forcing all derefs to be a chain of hlsl_deref ending in a var—which has
the advantage of letting weird LHS constructions work "for free" (not
that it particularly matters either).
As an aside, I think that if a structure splitting pass does happen, we
would need for arbitrary instructions to be able to hold either a
hlsl_deref or an hlsl_ir_node for any source fields. I'm not actually
sure that makes either approach better, but it seems at least worth
thinking about.
Any opinions?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-d3dcompiler-Introduce-a-separate-structure-for-LHS-d.patch
Type: text/x-patch
Size: 11060 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200406/5e0f647a/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0100-d3dcompiler-Synthesize-a-variable-when-subscripting-.patch
Type: text/x-patch
Size: 3163 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200406/5e0f647a/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0101-d3dcompiler-Synthesize-a-variable-when-indexing-a-no.patch
Type: text/x-patch
Size: 7619 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200406/5e0f647a/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0102-d3dcompiler-Store-the-array-and-record-fields-of-str.patch
Type: text/x-patch
Size: 9350 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200406/5e0f647a/attachment-0007.bin>
More information about the wine-devel
mailing list