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@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@gmail.com wrote:
Signed-off-by: Zebediah Figura zfigura@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; structdiff --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?