On 4/14/20 2:23 AM, Matteo Bruni wrote:
On Tue, Apr 7, 2020 at 3:52 AM Zebediah Figura zfigura@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 :-/