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

[Zebediah Figura]

On 4/14/20 12:13 PM, Matteo Bruni wrote:
> On Tue, Apr 14, 2020 at 4:35 PM Zebediah Figura <zfigura at codeweavers.com> wrote:
>>
>> On 4/14/20 2:23 AM, Matteo Bruni wrote:
>>> On Tue, Apr 7, 2020 at 3:52 AM Zebediah Figura <zfigura at codeweavers.com> wrote:
>>>>
>>>> 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?
>>>
>>> Starting from the last point, yeah, I think we need to preserve the
>>> possibility of having a deref (rather than a whole var) as the
>>> "ultimate" node in any instruction. I don't think it matters for the
>>> issue at hand but it's something to keep in mind.
>>>
>>> With that out of the way... I do agree in general about the
>>> awkwardness of the current solution. I'd make it more explicit: it's
>>> fundamentally a matter of distinguishing lvalue derefs vs rvalue
>>> derefs (the former isn't really a "read", in terms of semantics and
>>> also e.g. liveness). If you mark lvalue derefs in some way you should
>>> be able to fix liveness properly, and probably avoid some other
>>> headaches down the line. They don't have to be two separate
>>> hlsl_ir_node_type necessarily, although that's an option.
>>
>> Sure. I think I prefer if they are, à la 0001, since it's easy to make
>> that constraint at parse time and I think it makes liveness analysis
>> much simpler/clearer (than e.g. setting a flag would).
>>
>>> Your patch 0001 basically does that and enforces that the LHS of an
>>> assignment is an lvalue, if maybe not as explicitly. Also, since in
>>> "old" HLSL there are no pointers, there shouldn't be many other
>>> interesting cases for lvalues aside from assignments, so it probably
>>> does the job.
>>> Unrelated note: I agree with the comment in the patch about leaving
>>> cleanup to a following pass. I found it's usually a lot easier to
>>> leave "tidying up" to separate optimization passes (even trivial ones)
>>> rather than trying to generate "perfect" code right away. Exceptions
>>> apply, but that seems to be a generally good idea.
>>>
>>> Patches 0100-0102 also certainly work. If you go with something like
>>> patch 0001 you won't need these now, but I suspect they will come in
>>> handy anyway, eventually.
>>>
>>
>> I'll take that as "no opinion", then :-/
> 
> Well, my opinion is that both are okay :P
> What I was trying to say is that probably 0001 (or something along
> those lines, e.g. I'd prefer "lvalue" rather than "dst" in naming) is
> a more specific improvement for the current issue. That makes the
> other patches unnecessary for the moment but I wouldn't be surprised
> if 0100 and 0101 will be helpful later on for different reasons.
> 

Sure, that makes sense. I think I'll go ahead with that approach, then.