[PATCH 1/7] d3dcompiler: Parse array types.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/d3dcompiler_43/d3dcompiler_private.h | 2 + dlls/d3dcompiler_43/hlsl.y | 61 ++++++++++++++++++++++- dlls/d3dcompiler_43/tests/hlsl_d3d9.c | 59 ++++++++++++++++++++++ dlls/d3dcompiler_43/utils.c | 4 +- 4 files changed, 122 insertions(+), 4 deletions(-) diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h index 2d8eba0b554..1a155736fd0 100644 --- a/dlls/d3dcompiler_43/d3dcompiler_private.h +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h @@ -1154,8 +1154,10 @@ void add_function_decl(struct wine_rb_tree *funcs, char *name, struct hlsl_ir_fu struct bwriter_shader *parse_hlsl_shader(const char *text, enum shader_type type, DWORD major, DWORD minor, const char *entrypoint, char **messages) DECLSPEC_HIDDEN; +const char *debug_base_type(const struct hlsl_type *type) DECLSPEC_HIDDEN; const char *debug_hlsl_type(const struct hlsl_type *type) DECLSPEC_HIDDEN; const char *debug_modifiers(DWORD modifiers) DECLSPEC_HIDDEN; +const char *debug_node_type(enum hlsl_ir_node_type type) DECLSPEC_HIDDEN; void debug_dump_ir_function_decl(const struct hlsl_ir_function_decl *func) DECLSPEC_HIDDEN; void free_hlsl_type(struct hlsl_type *type) DECLSPEC_HIDDEN; diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y index e46bdbade3f..46d815d0144 100644 --- a/dlls/d3dcompiler_43/hlsl.y +++ b/dlls/d3dcompiler_43/hlsl.y @@ -926,6 +926,47 @@ static struct list *make_list(struct hlsl_ir_node *node) return list; } +static unsigned int evaluate_array_dimension(struct hlsl_ir_node *node) +{ + if (node->data_type->type != HLSL_CLASS_SCALAR) + return 0; + + switch (node->type) + { + case HLSL_IR_CONSTANT: + { + struct hlsl_ir_constant *constant = constant_from_node(node); + + switch (constant->node.data_type->base_type) + { + case HLSL_TYPE_UINT: + return constant->v.value.u[0]; + case HLSL_TYPE_INT: + return constant->v.value.i[0]; + case HLSL_TYPE_FLOAT: + return constant->v.value.f[0]; + case HLSL_TYPE_DOUBLE: + return constant->v.value.d[0]; + case HLSL_TYPE_BOOL: + return constant->v.value.b[0]; + default: + WARN("Invalid type %s.\n", debug_base_type(constant->node.data_type)); + return 0; + } + } + case HLSL_IR_CONSTRUCTOR: + case HLSL_IR_DEREF: + case HLSL_IR_EXPR: + case HLSL_IR_SWIZZLE: + FIXME("Unhandled type %s.\n", debug_node_type(node->type)); + return 0; + case HLSL_IR_ASSIGNMENT: + default: + WARN("Invalid node type %s.\n", debug_node_type(node->type)); + return 0; + } +} + %} %locations @@ -1651,9 +1692,25 @@ array: /* Empty */ } | '[' expr ']' { - FIXME("Array.\n"); - $$ = 0; + unsigned int size = evaluate_array_dimension(node_from_list($2)); + free_instr_list($2); + + if (!size) + { + hlsl_report_message(get_location(&@2), HLSL_LEVEL_ERROR, + "array size is not a positive integer constant\n"); + YYABORT; + } + TRACE("Array size %u.\n", size); + + if (size > 65536) + { + hlsl_report_message(get_location(&@2), HLSL_LEVEL_ERROR, + "array size must be between 1 and 65536"); + YYABORT; + } + $$ = size; } var_modifiers: /* Empty */ diff --git a/dlls/d3dcompiler_43/tests/hlsl_d3d9.c b/dlls/d3dcompiler_43/tests/hlsl_d3d9.c index c673e2be327..525de9910be 100644 --- a/dlls/d3dcompiler_43/tests/hlsl_d3d9.c +++ b/dlls/d3dcompiler_43/tests/hlsl_d3d9.c @@ -654,10 +654,47 @@ static void test_return(void) release_test_context(&test_context); } +static void test_array_dimensions(void) +{ + struct test_context test_context; + ID3D10Blob *ps_code = NULL; + struct vec4 v; + + static const char shader[] = + "float4 main(float x : TEXCOORD0) : COLOR\n" + "{\n" + " const int dim = 4;\n" + " float a[2 * 2] = {0.1, 0.2, 0.3, 0.4};\n" + " float b[4.1] = a;\n" + " float c[dim] = b;\n" + " float d[true] = {c[0]};\n" + " float e[65536];\n" + " return float4(d[0], c[0], c[1], c[3]);\n" + "}"; + + if (!init_test_context(&test_context)) + return; + + todo_wine ps_code = compile_shader(shader, "ps_2_0"); + if (ps_code) + { + draw_quad(test_context.device, ps_code); + + v = get_color_vec4(test_context.device, 0, 0); + ok(compare_vec4(&v, 0.1f, 0.1f, 0.2f, 0.4f, 0), + "Got unexpected value {%.8e, %.8e, %.8e, %.8e}.\n", v.x, v.y, v.z, v.w); + + ID3D10Blob_Release(ps_code); + } + + release_test_context(&test_context); +} + static void test_fail(void) { static const char *tests[] = { + /* 0 */ "float4 test() : SV_TARGET\n" "{\n" " return y;\n" @@ -689,6 +726,7 @@ static void test_fail(void) " return float4(x.x, x.y, 0, 0);\n" "}", + /* 5 */ "float4 test() : SV_TARGET\n" "{\n" " struct { int b,c; } x = {0};\n" @@ -715,6 +753,26 @@ static void test_fail(void) "{\n" " return pos;\n" "}", + + /* 10 */ + "float4 test(float2 pos: TEXCOORD0) : SV_TARGET\n" + "{\n" + " float a[0];\n" + " return float4(0, 0, 0, 0);\n" + "}", + + "float4 test(float2 pos: TEXCOORD0) : SV_TARGET\n" + "{\n" + " float a[65537];\n" + " return float4(0, 0, 0, 0);\n" + "}", + + "float4 test(float2 pos: TEXCOORD0) : SV_TARGET\n" + "{\n" + " int x;\n" + " float a[(x = 2)];\n" + " return float4(0, 0, 0, 0);\n" + "}", }; static const char *targets[] = {"ps_2_0", "ps_3_0", "ps_4_0"}; @@ -776,5 +834,6 @@ START_TEST(hlsl_d3d9) test_trig(); test_comma(); test_return(); + test_array_dimensions(); test_fail(); } diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c index 5eaff1730b3..fe2d23b623e 100644 --- a/dlls/d3dcompiler_43/utils.c +++ b/dlls/d3dcompiler_43/utils.c @@ -1652,7 +1652,7 @@ void init_functions_tree(struct wine_rb_tree *funcs) wine_rb_init(&hlsl_ctx.functions, compare_function_rb); } -static const char *debug_base_type(const struct hlsl_type *type) +const char *debug_base_type(const struct hlsl_type *type) { const char *name = "(unknown)"; @@ -1744,7 +1744,7 @@ const char *debug_modifiers(DWORD modifiers) return wine_dbg_sprintf("%s", string[0] ? string + 1 : ""); } -static const char *debug_node_type(enum hlsl_ir_node_type type) +const char *debug_node_type(enum hlsl_ir_node_type type) { static const char * const names[] = { -- 2.25.1
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/d3dcompiler_43/d3dcompiler_private.h | 2 +- dlls/d3dcompiler_43/hlsl.y | 8 +++----- dlls/d3dcompiler_43/utils.c | 3 ++- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h index 1a155736fd0..04878c70463 100644 --- a/dlls/d3dcompiler_43/d3dcompiler_private.h +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h @@ -1144,7 +1144,7 @@ struct hlsl_ir_node *implicit_conversion(struct hlsl_ir_node *node, struct hlsl_ struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var) DECLSPEC_HIDDEN; struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record, struct hlsl_struct_field *field) DECLSPEC_HIDDEN; struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *left, enum parse_assign_op assign_op, - DWORD writemask, struct hlsl_ir_node *right) DECLSPEC_HIDDEN; + struct hlsl_ir_node *right) DECLSPEC_HIDDEN; void push_scope(struct hlsl_parse_ctx *ctx) DECLSPEC_HIDDEN; BOOL pop_scope(struct hlsl_parse_ctx *ctx) DECLSPEC_HIDDEN; struct hlsl_ir_function_decl *new_func_decl(struct hlsl_type *return_type, struct list *parameters) DECLSPEC_HIDDEN; diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y index 46d815d0144..591f755a7b2 100644 --- a/dlls/d3dcompiler_43/hlsl.y +++ b/dlls/d3dcompiler_43/hlsl.y @@ -542,7 +542,7 @@ static void struct_var_initializer(struct list *list, struct hlsl_ir_var *var, } deref->node.loc = node->loc; list_add_tail(list, &deref->node.entry); - assignment = make_assignment(&deref->node, ASSIGN_OP_ASSIGN, BWRITERSP_WRITEMASK_ALL, node); + assignment = make_assignment(&deref->node, ASSIGN_OP_ASSIGN, node); list_add_tail(list, &assignment->entry); } else @@ -688,8 +688,7 @@ static struct list *declare_vars(struct hlsl_type *basic_type, DWORD modifiers, deref = new_var_deref(var); list_add_tail(statements_list, &deref->node.entry); - assignment = make_assignment(&deref->node, ASSIGN_OP_ASSIGN, - BWRITERSP_WRITEMASK_ALL, v->initializer.args[0]); + assignment = make_assignment(&deref->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]); d3dcompiler_free(v->initializer.args); list_add_tail(statements_list, &assignment->entry); } @@ -2413,8 +2412,7 @@ assignment_expr: conditional_expr hlsl_report_message(get_location(&@2), HLSL_LEVEL_ERROR, "l-value is const"); YYABORT; } - if (!(instr = make_assignment(node_from_list($1), $2, - BWRITERSP_WRITEMASK_ALL, node_from_list($3)))) + if (!(instr = make_assignment(node_from_list($1), $2, node_from_list($3)))) YYABORT; instr->loc = get_location(&@2); $$ = append_binop($3, $1, instr); diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c index fe2d23b623e..1b572aa0aec 100644 --- a/dlls/d3dcompiler_43/utils.c +++ b/dlls/d3dcompiler_43/utils.c @@ -1423,9 +1423,10 @@ static enum hlsl_ir_expr_op op_from_assignment(enum parse_assign_op op) } struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *left, enum parse_assign_op assign_op, - DWORD writemask, struct hlsl_ir_node *right) + struct hlsl_ir_node *right) { struct hlsl_ir_assignment *assign = d3dcompiler_alloc(sizeof(*assign)); + DWORD writemask = BWRITERSP_WRITEMASK_ALL; struct hlsl_type *type; struct hlsl_ir_node *lhs, *rhs; -- 2.25.1
Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com>
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/d3dcompiler_43/tests/hlsl_d3d9.c | 40 +++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/dlls/d3dcompiler_43/tests/hlsl_d3d9.c b/dlls/d3dcompiler_43/tests/hlsl_d3d9.c index 525de9910be..ce63d3ea5da 100644 --- a/dlls/d3dcompiler_43/tests/hlsl_d3d9.c +++ b/dlls/d3dcompiler_43/tests/hlsl_d3d9.c @@ -307,6 +307,21 @@ static void test_swizzle(void) " return ret;\n" "}"; + static const char ps_multiple_lhs_source[] = + "float4 main() : COLOR\n" + "{\n" + " float4 ret = float4(0.1, 0.2, 0.3, 0.4);\n" + " ret.wyz.yx = float2(0.5, 0.6).yx;\n" + " return ret;\n" + "}"; + + static const char ps_multiple_rhs_source[] = + "float4 main() : COLOR\n" + "{\n" + " float4 ret = float4(0.1, 0.2, 0.3, 0.4).ywxz.zyyz;\n" + " return ret;\n" + "}"; + if (!init_test_context(&test_context)) return; device = test_context.device; @@ -328,6 +343,31 @@ static void test_swizzle(void) ID3D10Blob_Release(ps_code); } + + todo_wine ps_code = compile_shader(ps_multiple_lhs_source, "ps_2_0"); + if (ps_code) + { + draw_quad(device, ps_code); + + v = get_color_vec4(device, 0, 0); + ok(compare_vec4(&v, 0.1f, 0.6f, 0.3f, 0.5f, 0), + "Got unexpected value {%.8e, %.8e, %.8e, %.8e}.\n", v.x, v.y, v.z, v.w); + + ID3D10Blob_Release(ps_code); + } + + todo_wine ps_code = compile_shader(ps_multiple_rhs_source, "ps_2_0"); + if (ps_code) + { + draw_quad(device, ps_code); + + v = get_color_vec4(device, 0, 0); + ok(compare_vec4(&v, 0.1f, 0.4f, 0.4f, 0.1f, 0), + "Got unexpected value {%.8e, %.8e, %.8e, %.8e}.\n", v.x, v.y, v.z, v.w); + + ID3D10Blob_Release(ps_code); + } + release_test_context(&test_context); } -- 2.25.1
Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com>
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/d3dcompiler_43/tests/hlsl_d3d9.c | 5 +- dlls/d3dcompiler_43/utils.c | 104 +++++++++++++++++++++----- 2 files changed, 88 insertions(+), 21 deletions(-) diff --git a/dlls/d3dcompiler_43/tests/hlsl_d3d9.c b/dlls/d3dcompiler_43/tests/hlsl_d3d9.c index ce63d3ea5da..7f391833f39 100644 --- a/dlls/d3dcompiler_43/tests/hlsl_d3d9.c +++ b/dlls/d3dcompiler_43/tests/hlsl_d3d9.c @@ -828,10 +828,9 @@ static void test_fail(void) compiled = errors = NULL; hr = ppD3DCompile(tests[i], strlen(tests[i]), NULL, NULL, NULL, "test", targets[j], 0, 0, &compiled, &errors); todo_wine ok(hr == E_FAIL, "Test %u, target %s, got unexpected hr %#x.\n", i, targets[j], hr); - todo_wine_if (i == 1) ok(!!errors, "Test %u, target %s, expected non-NULL error blob.\n", i, targets[j]); + ok(!!errors, "Test %u, target %s, expected non-NULL error blob.\n", i, targets[j]); ok(!compiled, "Test %u, target %s, expected no compiled shader blob.\n", i, targets[j]); - if (errors) - ID3D10Blob_Release(errors); + ID3D10Blob_Release(errors); } } } diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c index 1b572aa0aec..9c388008081 100644 --- a/dlls/d3dcompiler_43/utils.c +++ b/dlls/d3dcompiler_43/utils.c @@ -1422,13 +1422,46 @@ static enum hlsl_ir_expr_op op_from_assignment(enum parse_assign_op op) return ops[op]; } -struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *left, enum parse_assign_op assign_op, - struct hlsl_ir_node *right) +static unsigned int invert_swizzle(unsigned int *swizzle, unsigned int writemask) +{ + unsigned int i, j, bit = 0, inverted = 0, components, new_writemask = 0, new_swizzle = 0; + + /* Apply the writemask to the swizzle to get a new writemask and swizzle. */ + for (i = 0; i < 4; ++i) + { + if (writemask & (1 << i)) + { + unsigned int s = (*swizzle >> (i * 2)) & 3; + new_swizzle |= s << (bit++ * 2); + if (new_writemask & (1 << s)) + return 0; + new_writemask |= 1 << s; + } + } + components = bit; + + /* Invert the swizzle. */ + bit = 0; + for (i = 0; i < 4; ++i) + { + for (j = 0; j < components; ++j) + { + unsigned int s = (new_swizzle >> (j * 2)) & 3; + if (s == i) + inverted |= j << (bit++ * 2); + } + } + + *swizzle = inverted; + return new_writemask; +} + +struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *lhs, enum parse_assign_op assign_op, + struct hlsl_ir_node *rhs) { struct hlsl_ir_assignment *assign = d3dcompiler_alloc(sizeof(*assign)); - DWORD writemask = BWRITERSP_WRITEMASK_ALL; + DWORD writemask = (1 << lhs->data_type->dimx) - 1; struct hlsl_type *type; - struct hlsl_ir_node *lhs, *rhs; if (!assign) { @@ -1436,46 +1469,81 @@ struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *left, enum parse_assig return NULL; } + while (lhs->type != HLSL_IR_DEREF) + { + struct hlsl_ir_node *lhs_inner; + + if (lhs->type == HLSL_IR_EXPR && expr_from_node(lhs)->op == HLSL_IR_UNOP_CAST) + { + FIXME("Cast on the lhs.\n"); + d3dcompiler_free(assign); + return NULL; + } + else if (lhs->type == HLSL_IR_SWIZZLE) + { + struct hlsl_ir_swizzle *swizzle = swizzle_from_node(lhs); + + if (lhs->data_type->type == HLSL_CLASS_MATRIX) + FIXME("Assignments with writemasks and matrices on lhs are not supported yet.\n"); + + lhs_inner = swizzle->val; + list_remove(&lhs->entry); + + list_add_after(&rhs->entry, &lhs->entry); + swizzle->val = rhs; + if (!(writemask = invert_swizzle(&swizzle->swizzle, writemask))) + { + hlsl_report_message(lhs->loc, HLSL_LEVEL_ERROR, "invalid writemask"); + d3dcompiler_free(assign); + return NULL; + } + rhs = &swizzle->node; + } + else + { + hlsl_report_message(lhs->loc, HLSL_LEVEL_ERROR, "invalid lvalue"); + d3dcompiler_free(assign); + return NULL; + } + + lhs = lhs_inner; + } + TRACE("Creating proper assignment expression.\n"); - rhs = right; if (writemask == BWRITERSP_WRITEMASK_ALL) - type = left->data_type; + type = lhs->data_type; else { unsigned int dimx = 0; DWORD bitmask; enum hlsl_type_class type_class; - if (left->data_type->type > HLSL_CLASS_LAST_NUMERIC) + if (lhs->data_type->type > HLSL_CLASS_LAST_NUMERIC) { - hlsl_report_message(left->loc, HLSL_LEVEL_ERROR, + hlsl_report_message(lhs->loc, HLSL_LEVEL_ERROR, "writemask on a non scalar/vector/matrix type"); d3dcompiler_free(assign); return NULL; } - bitmask = writemask & ((1 << left->data_type->dimx) - 1); + bitmask = writemask & ((1 << lhs->data_type->dimx) - 1); while (bitmask) { if (bitmask & 1) dimx++; bitmask >>= 1; } - if (left->data_type->type == HLSL_CLASS_MATRIX) + if (lhs->data_type->type == HLSL_CLASS_MATRIX) FIXME("Assignments with writemasks and matrices on lhs are not supported yet.\n"); if (dimx == 1) type_class = HLSL_CLASS_SCALAR; else - type_class = left->data_type->type; - type = new_hlsl_type(NULL, type_class, left->data_type->base_type, dimx, 1); + type_class = lhs->data_type->type; + type = new_hlsl_type(NULL, type_class, lhs->data_type->base_type, dimx, 1); } assign->node.type = HLSL_IR_ASSIGNMENT; - assign->node.loc = left->loc; + assign->node.loc = lhs->loc; assign->node.data_type = type; assign->writemask = writemask; - FIXME("Check for casts in the lhs.\n"); - - lhs = left; - /* FIXME: check for invalid writemasks on the lhs. */ rhs = implicit_conversion(rhs, type, &rhs->loc); @@ -1485,7 +1553,7 @@ struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *left, enum parse_assig enum hlsl_ir_expr_op op = op_from_assignment(assign_op); struct hlsl_ir_node *expr; - if (lhs->type != HLSL_IR_DEREF || deref_from_node(lhs)->type != HLSL_IR_DEREF_VAR) + if (deref_from_node(lhs)->type != HLSL_IR_DEREF_VAR) { FIXME("LHS expression not supported in compound assignments yet.\n"); assign->rhs = rhs; -- 2.25.1
Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com> --- I stared at invert_swizzle() an inordinate amount of time and I eventually convinced myself it's correct, or at least sensible. I'm going to require more swizzle tests when the compiler will actually start to work. I assume you already have a bunch for this but, obviously, tests with multiple LHS swizzles would be particularly interesting.
On 3/12/20 8:05 AM, Matteo Bruni wrote:
Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com> --- I stared at invert_swizzle() an inordinate amount of time and I eventually convinced myself it's correct, or at least sensible. I'm going to require more swizzle tests when the compiler will actually start to work. I assume you already have a bunch for this but, obviously, tests with multiple LHS swizzles would be particularly interesting.
I'll admit I'm not happy about the code at all, but it's a complex operation and I haven't proven skilled enough to write anything better. I'll at least add some more tests.
On Thu, Mar 12, 2020, 15:19 Zebediah Figura <zfigura(a)codeweavers.com> wrote:
On 3/12/20 8:05 AM, Matteo Bruni wrote:
Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com> --- I stared at invert_swizzle() an inordinate amount of time and I eventually convinced myself it's correct, or at least sensible. I'm going to require more swizzle tests when the compiler will actually start to work. I assume you already have a bunch for this but, obviously, tests with multiple LHS swizzles would be particularly interesting.
I'll admit I'm not happy about the code at all, but it's a complex operation and I haven't proven skilled enough to write anything better.
I couldn't find any way to make it simpler either, so that makes two of us ;)
I'll at least add some more tests.
I'm going to trust them more than my review anyway.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/d3dcompiler_43/d3dcompiler_private.h | 25 ++++++++++++++--------- dlls/d3dcompiler_43/hlsl.y | 6 +++--- dlls/d3dcompiler_43/utils.c | 25 ++++++++++++++--------- 3 files changed, 33 insertions(+), 23 deletions(-) diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h index 04878c70463..830434c9ff1 100644 --- a/dlls/d3dcompiler_43/d3dcompiler_private.h +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h @@ -796,14 +796,6 @@ struct hlsl_ir_loop struct list *body; }; -struct hlsl_ir_assignment -{ - struct hlsl_ir_node node; - struct hlsl_ir_node *lhs; - struct hlsl_ir_node *rhs; - unsigned char writemask; -}; - enum hlsl_ir_expr_op { HLSL_IR_UNOP_BIT_NOT = 0, HLSL_IR_UNOP_LOGIC_NOT, @@ -907,9 +899,8 @@ enum hlsl_ir_deref_type HLSL_IR_DEREF_RECORD, }; -struct hlsl_ir_deref +struct hlsl_deref { - struct hlsl_ir_node node; enum hlsl_ir_deref_type type; union { @@ -927,6 +918,20 @@ struct hlsl_ir_deref } v; }; +struct hlsl_ir_deref +{ + struct hlsl_ir_node node; + struct hlsl_deref src; +}; + +struct hlsl_ir_assignment +{ + struct hlsl_ir_node node; + struct hlsl_deref lhs; + struct hlsl_ir_node *rhs; + unsigned char writemask; +}; + struct hlsl_ir_constant { struct hlsl_ir_node node; diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y index 591f755a7b2..a24d29e5df8 100644 --- a/dlls/d3dcompiler_43/hlsl.y +++ b/dlls/d3dcompiler_43/hlsl.y @@ -2123,9 +2123,9 @@ postfix_expr: primary_expr free_instr_list($3); YYABORT; } - deref->type = HLSL_IR_DEREF_ARRAY; - deref->v.array.array = node_from_list($1); - deref->v.array.index = node_from_list($3); + deref->src.type = HLSL_IR_DEREF_ARRAY; + deref->src.v.array.array = node_from_list($1); + deref->src.v.array.index = node_from_list($3); $$ = append_binop($1, $3, &deref->node); } diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c index 9c388008081..29b35c3d7eb 100644 --- a/dlls/d3dcompiler_43/utils.c +++ b/dlls/d3dcompiler_43/utils.c @@ -1380,8 +1380,8 @@ struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var) } deref->node.type = HLSL_IR_DEREF; deref->node.data_type = var->data_type; - deref->type = HLSL_IR_DEREF_VAR; - deref->v.var = var; + deref->src.type = HLSL_IR_DEREF_VAR; + deref->src.v.var = var; return deref; } @@ -1396,9 +1396,9 @@ struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record, struct hlsl_ } deref->node.type = HLSL_IR_DEREF; deref->node.data_type = field->type; - deref->type = HLSL_IR_DEREF_RECORD; - deref->v.record.record = record; - deref->v.record.field = field; + deref->src.type = HLSL_IR_DEREF_RECORD; + deref->src.v.record.record = record; + deref->src.v.record.field = field; return deref; } @@ -1547,13 +1547,13 @@ struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *lhs, enum parse_assign rhs = implicit_conversion(rhs, type, &rhs->loc); - assign->lhs = lhs; + assign->lhs = deref_from_node(lhs)->src; if (assign_op != ASSIGN_OP_ASSIGN) { enum hlsl_ir_expr_op op = op_from_assignment(assign_op); struct hlsl_ir_node *expr; - if (deref_from_node(lhs)->type != HLSL_IR_DEREF_VAR) + if (assign->lhs.type != HLSL_IR_DEREF_VAR) { FIXME("LHS expression not supported in compound assignments yet.\n"); assign->rhs = rhs; @@ -1567,7 +1567,12 @@ struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *lhs, enum parse_assign } } else + { + list_remove(&lhs->entry); + /* Don't recursively free the deref; we just copied its members. */ + d3dcompiler_free(lhs); assign->rhs = rhs; + } return &assign->node; } @@ -1860,7 +1865,7 @@ static void debug_dump_ir_var(const struct hlsl_ir_var *var) wine_dbg_printf(" : %s", debugstr_a(var->semantic)); } -static void debug_dump_ir_deref(const struct hlsl_ir_deref *deref) +static void debug_dump_deref(const struct hlsl_deref *deref) { switch (deref->type) { @@ -2048,7 +2053,7 @@ static const char *debug_writemask(DWORD writemask) static void debug_dump_ir_assignment(const struct hlsl_ir_assignment *assign) { wine_dbg_printf("= ("); - debug_dump_src(assign->lhs); + debug_dump_deref(&assign->lhs); if (assign->writemask != BWRITERSP_WRITEMASK_ALL) wine_dbg_printf("%s", debug_writemask(assign->writemask)); wine_dbg_printf(" "); @@ -2129,7 +2134,7 @@ static void debug_dump_instr(const struct hlsl_ir_node *instr) debug_dump_ir_expr(expr_from_node(instr)); break; case HLSL_IR_DEREF: - debug_dump_ir_deref(deref_from_node(instr)); + debug_dump_deref(&deref_from_node(instr)->src); break; case HLSL_IR_CONSTANT: debug_dump_ir_constant(constant_from_node(instr)); -- 2.25.1
Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com> --- It looks like this also fixes compound assignments (the same deref node / instruction was shared between the assignment LHS and the "new" expression on the RHS), right?
On 3/12/20 8:05 AM, Matteo Bruni wrote:
Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com> --- It looks like this also fixes compound assignments (the same deref node / instruction was shared between the assignment LHS and the "new" expression on the RHS), right?
I believe that's correct, yes.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/d3dcompiler_43/d3dcompiler_private.h | 3 +- dlls/d3dcompiler_43/hlsl.y | 39 +++++++++++++++++++++-- dlls/d3dcompiler_43/utils.c | 33 +++++++++---------- 3 files changed, 52 insertions(+), 23 deletions(-) diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h index 830434c9ff1..d221d1056fa 100644 --- a/dlls/d3dcompiler_43/d3dcompiler_private.h +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h @@ -912,7 +912,7 @@ struct hlsl_deref } array; struct { - struct hlsl_ir_node *record; + struct hlsl_deref *record; struct hlsl_struct_field *field; } record; } v; @@ -1147,7 +1147,6 @@ struct hlsl_ir_expr *new_cast(struct hlsl_ir_node *node, struct hlsl_type *type, struct hlsl_ir_node *implicit_conversion(struct hlsl_ir_node *node, struct hlsl_type *type, struct source_location *loc) DECLSPEC_HIDDEN; struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var) DECLSPEC_HIDDEN; -struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record, struct hlsl_struct_field *field) DECLSPEC_HIDDEN; struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *left, enum parse_assign_op assign_op, struct hlsl_ir_node *right) DECLSPEC_HIDDEN; void push_scope(struct hlsl_parse_ctx *ctx) DECLSPEC_HIDDEN; diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y index a24d29e5df8..40159c60dcc 100644 --- a/dlls/d3dcompiler_43/hlsl.y +++ b/dlls/d3dcompiler_43/hlsl.y @@ -504,6 +504,31 @@ static struct hlsl_ir_jump *new_return(struct hlsl_ir_node *value, struct source return jump; } +static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_deref *record, struct hlsl_struct_field *field) +{ + struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref)); + + if (!deref) + { + ERR("Out of memory.\n"); + return NULL; + } + deref->node.type = HLSL_IR_DEREF; + deref->node.data_type = field->type; + deref->src.type = HLSL_IR_DEREF_RECORD; + if (!(deref->src.v.record.record = d3dcompiler_alloc(sizeof(*deref->src.v.record.record)))) + { + ERR("Out of memory.\n"); + d3dcompiler_free(deref); + return NULL; + } + *deref->src.v.record.record = record->src; + list_remove(&record->node.entry); + d3dcompiler_free(record); + deref->src.v.record.field = field; + return deref; +} + static void struct_var_initializer(struct list *list, struct hlsl_ir_var *var, struct parse_initializer *initializer) { @@ -534,7 +559,7 @@ static void struct_var_initializer(struct list *list, struct hlsl_ir_var *var, } if (components_count_type(field->type) == components_count_type(node->data_type)) { - deref = new_record_deref(&new_var_deref(var)->node, field); + deref = new_record_deref(new_var_deref(var), field); if (!deref) { ERR("Out of memory.\n"); @@ -2025,19 +2050,27 @@ postfix_expr: primary_expr { struct hlsl_ir_node *node = node_from_list($1); struct source_location loc; - loc = get_location(&@2); if (node->data_type->type == HLSL_CLASS_STRUCT) { struct hlsl_type *type = node->data_type; struct hlsl_struct_field *field; + struct hlsl_ir_deref *src; + + if (node->type != HLSL_IR_DEREF) + { + FIXME("Unimplemented subscript of node type %s.\n", + debug_node_type(node->type)); + YYABORT; + } + src = deref_from_node(node); $$ = NULL; LIST_FOR_EACH_ENTRY(field, type->e.elements, struct hlsl_struct_field, entry) { if (!strcmp($3, field->name)) { - struct hlsl_ir_deref *deref = new_record_deref(node, field); + struct hlsl_ir_deref *deref = new_record_deref(src, field); if (!deref) { diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c index 29b35c3d7eb..ded8d3d77e3 100644 --- a/dlls/d3dcompiler_43/utils.c +++ b/dlls/d3dcompiler_43/utils.c @@ -1385,23 +1385,6 @@ struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var) return deref; } -struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record, struct hlsl_struct_field *field) -{ - struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref)); - - if (!deref) - { - ERR("Out of memory.\n"); - return NULL; - } - deref->node.type = HLSL_IR_DEREF; - deref->node.data_type = field->type; - deref->src.type = HLSL_IR_DEREF_RECORD; - deref->src.v.record.record = record; - deref->src.v.record.field = field; - return deref; -} - static enum hlsl_ir_expr_op op_from_assignment(enum parse_assign_op op) { static const enum hlsl_ir_expr_op ops[] = @@ -1881,7 +1864,7 @@ static void debug_dump_deref(const struct hlsl_deref *deref) wine_dbg_printf("]"); break; case HLSL_IR_DEREF_RECORD: - debug_dump_src(deref->v.record.record); + debug_dump_deref(deref->v.record.record); wine_dbg_printf(".%s", debugstr_a(deref->v.record.field->name)); break; } @@ -2232,8 +2215,22 @@ static void free_ir_constant(struct hlsl_ir_constant *constant) d3dcompiler_free(constant); } +static void free_deref(struct hlsl_deref *deref) +{ + switch (deref->type) + { + case HLSL_IR_DEREF_RECORD: + free_deref(deref->v.record.record); + d3dcompiler_free(deref); + break; + default: + break; + } +} + static void free_ir_deref(struct hlsl_ir_deref *deref) { + free_deref(&deref->src); d3dcompiler_free(deref); } -- 2.25.1
Signed-off-by: Zebediah Figura <zfigura(a)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; struct diff --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); TRACE("Array dereference from type %s\n", debug_hlsl_type(expr_type)); if (!deref) @@ -2157,7 +2166,17 @@ postfix_expr: primary_expr YYABORT; } deref->src.type = HLSL_IR_DEREF_ARRAY; - deref->src.v.array.array = node_from_list($1); + if (!(deref->src.v.array.array = d3dcompiler_alloc(sizeof(*deref->src.v.array.array)))) + { + ERR("Out of memory.\n"); + d3dcompiler_free(deref); + free_instr_list($1); + free_instr_list($3); + YYABORT; + } + *deref->src.v.array.array = src->src; + list_remove(&src->node.entry); + d3dcompiler_free(src); deref->src.v.array.index = node_from_list($3); $$ = append_binop($1, $3, &deref->node); diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c index ded8d3d77e3..9ee392967ce 100644 --- a/dlls/d3dcompiler_43/utils.c +++ b/dlls/d3dcompiler_43/utils.c @@ -1858,7 +1858,7 @@ static void debug_dump_deref(const struct hlsl_deref *deref) wine_dbg_printf(")"); break; case HLSL_IR_DEREF_ARRAY: - debug_dump_src(deref->v.array.array); + debug_dump_deref(deref->v.array.array); wine_dbg_printf("["); debug_dump_src(deref->v.array.index); wine_dbg_printf("]"); @@ -2223,6 +2223,10 @@ static void free_deref(struct hlsl_deref *deref) free_deref(deref->v.record.record); d3dcompiler_free(deref); break; + case HLSL_IR_DEREF_ARRAY: + free_deref(deref->v.array.array); + d3dcompiler_free(deref); + break; default: break; } -- 2.25.1
On Sat, Mar 7, 2020 at 12:25 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
Signed-off-by: Zebediah Figura <zfigura(a)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; struct diff --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.
On 3/11/20 12:57 PM, Matteo Bruni wrote:
On Sat, Mar 7, 2020 at 12:25 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
Signed-off-by: Zebediah Figura <zfigura(a)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; struct diff --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().
On Wed, Mar 11, 2020 at 8:45 PM Zebediah Figura <zfigura(a)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(a)gmail.com> wrote:
Signed-off-by: Zebediah Figura <zfigura(a)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; struct diff --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) (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.
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.
On 3/12/20 6:58 AM, Matteo Bruni wrote:
On Wed, Mar 11, 2020 at 8:45 PM Zebediah Figura <zfigura(a)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(a)gmail.com> wrote:
Signed-off-by: Zebediah Figura <zfigura(a)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; struct diff --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.
On Thu, Mar 12, 2020 at 4:12 PM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
On 3/12/20 6:58 AM, Matteo Bruni wrote:
On Wed, Mar 11, 2020 at 8:45 PM Zebediah Figura <zfigura(a)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(a)gmail.com> wrote:
Signed-off-by: Zebediah Figura <zfigura(a)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; struct diff --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...
Sure, no reason to add more machinery until necessary.
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(a)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(a)gmail.com> wrote:
Signed-off-by: Zebediah Figura <zfigura(a)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; struct diff --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?
On Tue, Apr 7, 2020 at 3:52 AM Zebediah Figura <zfigura(a)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. 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.
On 4/14/20 2:23 AM, Matteo Bruni wrote:
On Tue, Apr 7, 2020 at 3:52 AM Zebediah Figura <zfigura(a)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 :-/
On Tue, Apr 14, 2020 at 4:35 PM Zebediah Figura <zfigura(a)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(a)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.
On 4/14/20 12:13 PM, Matteo Bruni wrote:
On Tue, Apr 14, 2020 at 4:35 PM Zebediah Figura <zfigura(a)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(a)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.
Sure, that makes sense. I think I'll go ahead with that approach, then.
participants (4)
-
Matteo Bruni -
Matteo Bruni -
Zebediah Figura -
Zebediah Figura