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

[Zebediah Figura]

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 :-/