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.
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().