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

[Matteo Bruni]

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.