[PATCH 0/3] MR33: vkd3d-shader/hlsl: Support complex implicit casts, complex explicit casts and complex broadcasts. (PART 2/3)
Continuation of the patch series to support: * Complex broadcasts. * Complex implicit casts between component-wise equal types. * Complex explicit casts between component-wise compatible types. By Zeb's suggestion, I added tests for explicit casts between structs and vectors and arrays and vectors. This was helpful for catching special edge cases. I realized that the natural thing to do would be to also include tests for explicit casts between matrices and structs, matrices and arrays, and matrices and vectors. This made the patch series larger so I split it again (that's why this is PART 2/3). Following patches in: https://gitlab.winehq.org/fcasas/vkd3d/-/commits/complex_broadcasts_2/ -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/33
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 03004c60..c8bf75e7 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -148,14 +148,27 @@ static void check_invalid_matrix_modifiers(struct hlsl_ctx *ctx, DWORD modifiers "'row_major' and 'column_major' modifiers are only allowed for matrices."); } -static bool convertible_data_type(struct hlsl_type *type) +static bool type_contains_only_numerics(struct hlsl_type *type) { - return type->type != HLSL_CLASS_OBJECT; + unsigned int i; + + if (type->type == HLSL_CLASS_ARRAY) + return type_contains_only_numerics(type->e.array.type); + if (type->type == HLSL_CLASS_STRUCT) + { + for (i = 0; i < type->e.record.field_count; ++i) + { + if (!type_contains_only_numerics(type->e.record.fields[i].type)) + return false; + } + return true; + } + return type->type <= HLSL_CLASS_LAST_NUMERIC; } static bool compatible_data_types(struct hlsl_type *t1, struct hlsl_type *t2) { - if (!convertible_data_type(t1) || !convertible_data_type(t2)) + if (!type_contains_only_numerics(t1) || !type_contains_only_numerics(t2)) return false; if (t1->type <= HLSL_CLASS_LAST_NUMERIC) @@ -209,7 +222,7 @@ static bool compatible_data_types(struct hlsl_type *t1, struct hlsl_type *t2) static bool implicit_compatible_data_types(struct hlsl_type *t1, struct hlsl_type *t2) { - if (!convertible_data_type(t1) || !convertible_data_type(t2)) + if (!type_contains_only_numerics(t1) || !type_contains_only_numerics(t2)) return false; if (t1->type <= HLSL_CLASS_LAST_NUMERIC) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/33
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 156 +++++++++++++-------- tests/cast-componentwise-equal.shader_test | 16 +-- 2 files changed, 109 insertions(+), 63 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index c8bf75e7..90bbc75c 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -148,6 +148,26 @@ static void check_invalid_matrix_modifiers(struct hlsl_ctx *ctx, DWORD modifiers "'row_major' and 'column_major' modifiers are only allowed for matrices."); } +static bool hlsl_types_are_componentwise_equal(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct hlsl_type *t2) +{ + unsigned int k, count = hlsl_type_component_count(t1); + + if (count != hlsl_type_component_count(t2)) + return false; + + for (k = 0; k < count; ++k) + { + struct hlsl_type *t1_comp_type, *t2_comp_type; + + t1_comp_type = hlsl_type_get_component_type(ctx, t1, k); + t2_comp_type = hlsl_type_get_component_type(ctx, t2, k); + + if (!hlsl_types_are_equal(t1_comp_type, t2_comp_type)) + return false; + } + return true; +} + static bool type_contains_only_numerics(struct hlsl_type *type) { unsigned int i; @@ -220,67 +240,67 @@ static bool compatible_data_types(struct hlsl_type *t1, struct hlsl_type *t2) return false; } -static bool implicit_compatible_data_types(struct hlsl_type *t1, struct hlsl_type *t2) +static bool implicit_compatible_data_types(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct hlsl_type *t2) { - if (!type_contains_only_numerics(t1) || !type_contains_only_numerics(t2)) - return false; - - if (t1->type <= HLSL_CLASS_LAST_NUMERIC) + if (type_contains_only_numerics(t1) && type_contains_only_numerics(t2)) { - /* Scalar vars can be converted to any other numeric data type */ - if (t1->dimx == 1 && t1->dimy == 1 && t2->type <= HLSL_CLASS_LAST_NUMERIC) - return true; - /* The other way around is true too */ - if (t2->dimx == 1 && t2->dimy == 1 && t2->type <= HLSL_CLASS_LAST_NUMERIC) - return true; - } - - if (t1->type == HLSL_CLASS_ARRAY && t2->type == HLSL_CLASS_ARRAY) - { - return hlsl_type_component_count(t1) == hlsl_type_component_count(t2); - } - - if ((t1->type == HLSL_CLASS_ARRAY && t2->type <= HLSL_CLASS_LAST_NUMERIC) - || (t1->type <= HLSL_CLASS_LAST_NUMERIC && t2->type == HLSL_CLASS_ARRAY)) - { - /* e.g. float4[3] to float4 is allowed */ - if (t1->type == HLSL_CLASS_ARRAY && hlsl_types_are_equal(t1->e.array.type, t2)) - return true; - if (hlsl_type_component_count(t1) == hlsl_type_component_count(t2)) - return true; - return false; - } - - if (t1->type <= HLSL_CLASS_VECTOR && t2->type <= HLSL_CLASS_VECTOR) - { - if (t1->dimx >= t2->dimx) - return true; - return false; - } + if (t1->type <= HLSL_CLASS_LAST_NUMERIC) + { + /* Scalar vars can be converted to any other numeric data type */ + if (t1->dimx == 1 && t1->dimy == 1 && t2->type <= HLSL_CLASS_LAST_NUMERIC) + return true; + /* The other way around is true too */ + if (t2->dimx == 1 && t2->dimy == 1 && t2->type <= HLSL_CLASS_LAST_NUMERIC) + return true; + } - if (t1->type == HLSL_CLASS_MATRIX || t2->type == HLSL_CLASS_MATRIX) - { - if (t1->type == HLSL_CLASS_MATRIX && t2->type == HLSL_CLASS_MATRIX) - return t1->dimx >= t2->dimx && t1->dimy >= t2->dimy; + if (t1->type == HLSL_CLASS_ARRAY && t2->type == HLSL_CLASS_ARRAY) + { + return hlsl_type_component_count(t1) == hlsl_type_component_count(t2); + } - /* Matrix-vector conversion is apparently allowed if they have - * the same components count, or if the matrix is 1xN or Nx1 - * and we are reducing the component count */ - if (t1->type == HLSL_CLASS_VECTOR || t2->type == HLSL_CLASS_VECTOR) + if ((t1->type == HLSL_CLASS_ARRAY && t2->type <= HLSL_CLASS_LAST_NUMERIC) + || (t1->type <= HLSL_CLASS_LAST_NUMERIC && t2->type == HLSL_CLASS_ARRAY)) { + /* e.g. float4[3] to float4 is allowed */ + if (t1->type == HLSL_CLASS_ARRAY && hlsl_types_are_equal(t1->e.array.type, t2)) + return true; if (hlsl_type_component_count(t1) == hlsl_type_component_count(t2)) return true; + return false; + } - if ((t1->type == HLSL_CLASS_VECTOR || t1->dimx == 1 || t1->dimy == 1) && - (t2->type == HLSL_CLASS_VECTOR || t2->dimx == 1 || t2->dimy == 1)) - return hlsl_type_component_count(t1) >= hlsl_type_component_count(t2); + if (t1->type <= HLSL_CLASS_VECTOR && t2->type <= HLSL_CLASS_VECTOR) + { + if (t1->dimx >= t2->dimx) + return true; + return false; } - return false; + if (t1->type == HLSL_CLASS_MATRIX || t2->type == HLSL_CLASS_MATRIX) + { + if (t1->type == HLSL_CLASS_MATRIX && t2->type == HLSL_CLASS_MATRIX) + return t1->dimx >= t2->dimx && t1->dimy >= t2->dimy; + + /* Matrix-vector conversion is apparently allowed if they have + * the same components count, or if the matrix is 1xN or Nx1 + * and we are reducing the component count */ + if (t1->type == HLSL_CLASS_VECTOR || t2->type == HLSL_CLASS_VECTOR) + { + if (hlsl_type_component_count(t1) == hlsl_type_component_count(t2)) + return true; + + if ((t1->type == HLSL_CLASS_VECTOR || t1->dimx == 1 || t1->dimy == 1) && + (t2->type == HLSL_CLASS_VECTOR || t2->dimx == 1 || t2->dimy == 1)) + return hlsl_type_component_count(t1) >= hlsl_type_component_count(t2); + } + + return false; + } } - if (t1->type == HLSL_CLASS_STRUCT && t2->type == HLSL_CLASS_STRUCT) - return hlsl_types_are_equal(t1, t2); + if (t1->type > HLSL_CLASS_LAST_NUMERIC && t2->type > HLSL_CLASS_LAST_NUMERIC) + return hlsl_types_are_componentwise_equal(ctx, t1, t2); return false; } @@ -291,8 +311,14 @@ static struct hlsl_ir_load *add_load_component(struct hlsl_ctx *ctx, struct list static struct hlsl_ir_node *add_cast(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *node, struct hlsl_type *dst_type, const struct vkd3d_shader_location *loc) { + unsigned int dst_component_count = hlsl_type_component_count(dst_type); struct hlsl_type *src_type = node->data_type; + struct hlsl_deref var_deref; + struct hlsl_ir_store *store; struct hlsl_ir_expr *cast; + struct hlsl_ir_load *load; + struct hlsl_ir_var *var; + struct hlsl_block block; if (hlsl_types_are_equal(src_type, dst_type)) return node; @@ -300,9 +326,6 @@ static struct hlsl_ir_node *add_cast(struct hlsl_ctx *ctx, struct list *instrs, if ((src_type->type == HLSL_CLASS_MATRIX || dst_type->type == HLSL_CLASS_MATRIX) && src_type->type <= HLSL_CLASS_LAST_NUMERIC && dst_type->type <= HLSL_CLASS_LAST_NUMERIC) { - struct hlsl_deref var_deref; - struct hlsl_ir_load *load; - struct hlsl_ir_var *var; unsigned int dst_idx; bool broadcast; @@ -321,8 +344,6 @@ static struct hlsl_ir_node *add_cast(struct hlsl_ctx *ctx, struct list *instrs, for (dst_idx = 0; dst_idx < dst_type->dimx * dst_type->dimy; ++dst_idx) { struct hlsl_type *dst_scalar_type; - struct hlsl_ir_store *store; - struct hlsl_block block; unsigned int src_idx; if (broadcast) @@ -363,6 +384,31 @@ static struct hlsl_ir_node *add_cast(struct hlsl_ctx *ctx, struct list *instrs, return &load->node; } + else if ((src_type->type > HLSL_CLASS_LAST_NUMERIC || dst_type->type > HLSL_CLASS_LAST_NUMERIC) + && hlsl_types_are_componentwise_equal(ctx, src_type, dst_type)) + { + unsigned int k; + + if (!(var = hlsl_new_synthetic_var(ctx, "cast", dst_type, loc))) + return NULL; + hlsl_init_simple_deref_from_var(&var_deref, var); + + for (k = 0; k < dst_component_count; ++k) + { + if (!(load = add_load_component(ctx, instrs, node, k, loc))) + return NULL; + + if (!(store = hlsl_new_store_component(ctx, &block, &var_deref, k, &load->node))) + return NULL; + list_move_tail(instrs, &block.instrs); + } + + if (!(load = hlsl_new_var_load(ctx, var, *loc))) + return NULL; + list_add_tail(instrs, &load->node.entry); + + return &load->node; + } else { if (!(cast = hlsl_new_cast(ctx, node, dst_type, loc))) @@ -380,7 +426,7 @@ static struct hlsl_ir_node *add_implicit_conversion(struct hlsl_ctx *ctx, struct if (hlsl_types_are_equal(src_type, dst_type)) return node; - if (!implicit_compatible_data_types(src_type, dst_type)) + if (!implicit_compatible_data_types(ctx, src_type, dst_type)) { struct vkd3d_string_buffer *src_string, *dst_string; diff --git a/tests/cast-componentwise-equal.shader_test b/tests/cast-componentwise-equal.shader_test index ac329112..d0775f47 100644 --- a/tests/cast-componentwise-equal.shader_test +++ b/tests/cast-componentwise-equal.shader_test @@ -50,8 +50,8 @@ float4 main() : sv_target [test] -todo draw quad -todo probe all rgba (1.0, 2.0, 3.0, 1.0) +draw quad +probe all rgba (1.0, 2.0, 3.0, 1.0) [pixel shader fail] @@ -89,8 +89,8 @@ float4 main() : sv_target [test] -todo draw quad -todo probe all rgba (5.0, 6.0, 7.0, 8.0) +draw quad +probe all rgba (5.0, 6.0, 7.0, 8.0) [pixel shader] @@ -120,8 +120,8 @@ float4 main() : sv_target } [test] -todo draw quad -todo probe all rgba (4.0, 4.0, 4.0, 4.0) +draw quad +probe all rgba (4.0, 4.0, 4.0, 4.0) [pixel shader] @@ -199,8 +199,8 @@ float4 main() : sv_target } [test] -todo draw quad -todo probe all rgba (71.0, 73.0, 73.0, 74.0) +draw quad +probe all rgba (71.0, 73.0, 73.0, 74.0) [pixel shader fail] -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/33
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 110 +++++++++++------- .../cast-componentwise-compatible.shader_test | 20 ++-- 2 files changed, 78 insertions(+), 52 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 90bbc75c..4ae746ac 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -168,6 +168,27 @@ static bool hlsl_types_are_componentwise_equal(struct hlsl_ctx *ctx, struct hlsl return true; } +static bool hlsl_types_are_componentwise_compatible(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct hlsl_type *t2) +{ + unsigned int k, count = hlsl_type_component_count(t2); + + if (count > hlsl_type_component_count(t1)) + return false; + + for (k = 0; k < count; ++k) + { + struct hlsl_type *t1_comp_type, *t2_comp_type; + + t1_comp_type = hlsl_type_get_component_type(ctx, t1, k); + t2_comp_type = hlsl_type_get_component_type(ctx, t2, k); + + if ((t1_comp_type->type != HLSL_CLASS_SCALAR || t2_comp_type->type != HLSL_CLASS_SCALAR) + && !hlsl_types_are_equal(t1_comp_type, t2_comp_type)) + return false; + } + return true; +} + static bool type_contains_only_numerics(struct hlsl_type *type) { unsigned int i; @@ -186,58 +207,56 @@ static bool type_contains_only_numerics(struct hlsl_type *type) return type->type <= HLSL_CLASS_LAST_NUMERIC; } -static bool compatible_data_types(struct hlsl_type *t1, struct hlsl_type *t2) +static bool compatible_data_types(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct hlsl_type *t2) { - if (!type_contains_only_numerics(t1) || !type_contains_only_numerics(t2)) - return false; - - if (t1->type <= HLSL_CLASS_LAST_NUMERIC) + if (type_contains_only_numerics(t1) && type_contains_only_numerics(t2)) { - /* Scalar vars can be cast to pretty much everything */ - if (t1->dimx == 1 && t1->dimy == 1) + if (t1->type <= HLSL_CLASS_LAST_NUMERIC) + { + /* Scalar vars can be cast to pretty much everything */ + if (t1->dimx == 1 && t1->dimy == 1) + return true; + + if (t1->type == HLSL_CLASS_VECTOR && t2->type == HLSL_CLASS_VECTOR) + return t1->dimx >= t2->dimx; + } + + /* The other way around is true too i.e. whatever to scalar */ + if (t2->type <= HLSL_CLASS_LAST_NUMERIC && t2->dimx == 1 && t2->dimy == 1) return true; - if (t1->type == HLSL_CLASS_VECTOR && t2->type == HLSL_CLASS_VECTOR) - return t1->dimx >= t2->dimx; - } + if (t1->type == HLSL_CLASS_ARRAY) + { + if (hlsl_types_are_equal(t1->e.array.type, t2)) + /* e.g. float4[3] to float4 is allowed */ + return true; - /* The other way around is true too i.e. whatever to scalar */ - if (t2->type <= HLSL_CLASS_LAST_NUMERIC && t2->dimx == 1 && t2->dimy == 1) - return true; + if (t2->type == HLSL_CLASS_ARRAY || t2->type == HLSL_CLASS_STRUCT) + return hlsl_type_component_count(t1) >= hlsl_type_component_count(t2); + else + return hlsl_type_component_count(t1) == hlsl_type_component_count(t2); + } - if (t1->type == HLSL_CLASS_ARRAY) - { - if (hlsl_types_are_equal(t1->e.array.type, t2)) - /* e.g. float4[3] to float4 is allowed */ - return true; + if (t1->type == HLSL_CLASS_STRUCT) + return hlsl_type_component_count(t1) >= hlsl_type_component_count(t2); if (t2->type == HLSL_CLASS_ARRAY || t2->type == HLSL_CLASS_STRUCT) - return hlsl_type_component_count(t1) >= hlsl_type_component_count(t2); - else return hlsl_type_component_count(t1) == hlsl_type_component_count(t2); - } - - if (t1->type == HLSL_CLASS_STRUCT) - return hlsl_type_component_count(t1) >= hlsl_type_component_count(t2); - if (t2->type == HLSL_CLASS_ARRAY || t2->type == HLSL_CLASS_STRUCT) - return hlsl_type_component_count(t1) == hlsl_type_component_count(t2); - - if (t1->type == HLSL_CLASS_MATRIX || t2->type == HLSL_CLASS_MATRIX) - { - if (t1->type == HLSL_CLASS_MATRIX && t2->type == HLSL_CLASS_MATRIX && t1->dimx >= t2->dimx && t1->dimy >= t2->dimy) - return true; + if (t1->type == HLSL_CLASS_MATRIX || t2->type == HLSL_CLASS_MATRIX) + { + if (t1->type == HLSL_CLASS_MATRIX && t2->type == HLSL_CLASS_MATRIX && t1->dimx >= t2->dimx && t1->dimy >= t2->dimy) + return true; - /* Matrix-vector conversion is apparently allowed if they have the same components count */ - if ((t1->type == HLSL_CLASS_VECTOR || t2->type == HLSL_CLASS_VECTOR) - && hlsl_type_component_count(t1) == hlsl_type_component_count(t2)) - return true; - return false; + /* Matrix-vector conversion is apparently allowed if they have the same components count */ + if ((t1->type == HLSL_CLASS_VECTOR || t2->type == HLSL_CLASS_VECTOR) + && hlsl_type_component_count(t1) == hlsl_type_component_count(t2)) + return true; + return false; + } } - if (hlsl_type_component_count(t1) >= hlsl_type_component_count(t2)) - return true; - return false; + return hlsl_types_are_componentwise_compatible(ctx, t1, t2); } static bool implicit_compatible_data_types(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct hlsl_type *t2) @@ -385,8 +404,9 @@ static struct hlsl_ir_node *add_cast(struct hlsl_ctx *ctx, struct list *instrs, return &load->node; } else if ((src_type->type > HLSL_CLASS_LAST_NUMERIC || dst_type->type > HLSL_CLASS_LAST_NUMERIC) - && hlsl_types_are_componentwise_equal(ctx, src_type, dst_type)) + && dst_component_count <= hlsl_type_component_count(src_type)) { + struct hlsl_type *dst_comp_type; unsigned int k; if (!(var = hlsl_new_synthetic_var(ctx, "cast", dst_type, loc))) @@ -395,10 +415,16 @@ static struct hlsl_ir_node *add_cast(struct hlsl_ctx *ctx, struct list *instrs, for (k = 0; k < dst_component_count; ++k) { + dst_comp_type = hlsl_type_get_component_type(ctx, dst_type, k); + if (!(load = add_load_component(ctx, instrs, node, k, loc))) return NULL; - if (!(store = hlsl_new_store_component(ctx, &block, &var_deref, k, &load->node))) + if (!(cast = hlsl_new_cast(ctx, &load->node, dst_comp_type, loc))) + return NULL; + list_add_tail(instrs, &cast->node.entry); + + if (!(store = hlsl_new_store_component(ctx, &block, &var_deref, k, &cast->node))) return NULL; list_move_tail(instrs, &block.instrs); } @@ -4412,7 +4438,7 @@ unary_expr: dst_type = hlsl_new_array_type(ctx, dst_type, $4.sizes[i]); } - if (!compatible_data_types(src_type, dst_type)) + if (!compatible_data_types(ctx, src_type, dst_type)) { struct vkd3d_string_buffer *src_string, *dst_string; diff --git a/tests/cast-componentwise-compatible.shader_test b/tests/cast-componentwise-compatible.shader_test index c6402cf5..286c180b 100644 --- a/tests/cast-componentwise-compatible.shader_test +++ b/tests/cast-componentwise-compatible.shader_test @@ -16,8 +16,8 @@ float4 main() : sv_target [test] -todo draw quad -todo probe all rgba (1.0, 2.0, 3.0, 1.0) +draw quad +probe all rgba (1.0, 2.0, 3.0, 1.0) [pixel shader] @@ -39,8 +39,8 @@ float4 main() : sv_target [test] -todo draw quad -todo probe all rgba (5.0, 6.0, 7.0, 8.0) +draw quad +probe all rgba (5.0, 6.0, 7.0, 8.0) [pixel shader] @@ -55,8 +55,8 @@ float4 main() : sv_target [test] -todo draw quad -todo probe all rgba (1.0, 2.0, 3.0, 4.0) +draw quad +probe all rgba (1.0, 2.0, 3.0, 4.0) [pixel shader] @@ -86,8 +86,8 @@ float4 main() : sv_target } [test] -todo draw quad -todo probe all rgba (7.0, 7.0, 7.0, 7.0) +draw quad +probe all rgba (7.0, 7.0, 7.0, 7.0) [pixel shader] @@ -119,8 +119,8 @@ float4 main() : sv_target } [test] -todo draw quad -todo probe all rgba (3.0, 3.0, 3.0, 3.0) +draw quad +probe all rgba (3.0, 3.0, 3.0, 3.0) [pixel shader fail] -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/33
type_contains_only_numerics() (and/or the interaction with hlsl_types_are_componentwise_compatible()) felt weird to me, so I ended up going through and seeing if it's really necessary. implicit_compatible_data_types() handles the following cases: * 1x1 to/from vector/matrix — these are already numeric * array to array element — this actually shouldn't be allowed implicitly (currently broken). * array to/from vector/matrix — shouldn't be allowed implicitly (currently broken). * array to array — this should be allowed only if the types match (currently broken), and so should fall under hlsl_types_are_componentwise_compatible(). * vector to vector — already numeric * vector to/from matrix — already numeric compatible_data_types() handles the following cases: * 1x1 to anything — we do actually need type_contains_only_numerics() for this * numerics to 1x1 — already numeric * array to array element — this *is* legal with objects, and we shouldn't guard it with type_contains_only_numerics() * array to array/struct — same, and this should fall under hlsl_types_are_componentwise_compatible() * numeric to array/struct — should fall under hlsl_types_are_componentwise_compatible() I think? * matrix to matrix — already numeric The summary, and upshot, is (1) we only actually need type_contains_only_numerics() for (explicit!) scalar-to-struct/array broadcasts (2) we can replace some of the existing cases with hlsl_types_are_componentwise_{equal,compatible}() and thereby simplify (implicit)_compatible_data_types() a bit. We probably can defer some of the fixes (broken implicit conversions listed above) to after this patch, honestly, but if it's simple enough doing it first wouldn't be bad either. One other thing I'd request, as long as we're modifying this code—can we rename "t1" and "t2" to "src" and "dst", so this is easier to read? (Ideally put that commit first, too.) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/33#note_11241
Hello, On 19-10-22 01:52, Zebediah Figura (@zfigura) wrote:
type_contains_only_numerics() (and/or the interaction with hlsl_types_are_componentwise_compatible()) felt weird to me, so I ended up going through and seeing if it's really necessary.
implicit_compatible_data_types() handles the following cases:
* 1x1 to/from vector/matrix — these are already numeric * array to array element — this actually shouldn't be allowed implicitly (currently broken). * array to/from vector/matrix — shouldn't be allowed implicitly (currently broken). * array to array — this should be allowed only if the types match (currently broken), and so should fall under hlsl_types_are_componentwise_compatible(). * vector to vector — already numeric * vector to/from matrix — already numeric
Oh, interesting, I didn't realize that there were currently broken implicit casts, I simply assumed they were correct. I added implicit_compatible_data_types() for the sake of making it "more correct", but I will check this in more detail.
compatible_data_types() handles the following cases:
* 1x1 to anything — we do actually need type_contains_only_numerics() for this * numerics to 1x1 — already numeric * array to array element — this *is* legal with objects, and we shouldn't guard it with type_contains_only_numerics() * array to array/struct — same, and this should fall under hlsl_types_are_componentwise_compatible() * numeric to array/struct — should fall under hlsl_types_are_componentwise_compatible() I think? * matrix to matrix — already numeric
The summary, and upshot, is
(1) we only actually need type_contains_only_numerics() for (explicit!) scalar-to-struct/array broadcasts
(2) we can replace some of the existing cases with hlsl_types_are_componentwise_{equal,compatible}() and thereby simplify (implicit)_compatible_data_types() a bit.
I think I understand what you mean. I simplified compatible_data_types() in this patch (in the continuation of the series): https://gitlab.winehq.org/fcasas/vkd3d/-/commit/838e575fdde5b15cfde785250d84... but I didn't think I could do the same for implicit_compatible_data_types().
We probably can defer some of the fixes (broken implicit conversions listed above) to after this patch, honestly, but if it's simple enough doing it first wouldn't be bad either.
I will work on these fixes now.
One other thing I'd request, as long as we're modifying this code—can we rename "t1" and "t2" to "src" and "dst", so this is easier to read? (Ideally put that commit first, too.)
Okay.
On 10/19/22 07:57, Francisco Casas wrote:
Oh, interesting, I didn't realize that there were currently broken implicit casts, I simply assumed they were correct. I added implicit_compatible_data_types() for the sake of making it "more correct", but I will check this in more detail.
Heh, neither did I until I started testing some of the related corner cases :-)
compatible_data_types() handles the following cases:
* 1x1 to anything — we do actually need type_contains_only_numerics() for this * numerics to 1x1 — already numeric * array to array element — this *is* legal with objects, and we shouldn't guard it with type_contains_only_numerics() * array to array/struct — same, and this should fall under hlsl_types_are_componentwise_compatible() * numeric to array/struct — should fall under hlsl_types_are_componentwise_compatible() I think? * matrix to matrix — already numeric
The summary, and upshot, is
(1) we only actually need type_contains_only_numerics() for (explicit!) scalar-to-struct/array broadcasts
(2) we can replace some of the existing cases with hlsl_types_are_componentwise_{equal,compatible}() and thereby simplify (implicit)_compatible_data_types() a bit.
I think I understand what you mean. I simplified compatible_data_types() in this patch (in the continuation of the series):
https://gitlab.winehq.org/fcasas/vkd3d/-/commit/838e575fdde5b15cfde785250d84...
but I didn't think I could do the same for implicit_compatible_data_types().
Yeah. My take is that most of that simplification probably belongs in this patch rather than that one.
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
+ for (k = 0; k < dst_component_count; ++k) + { + if (!(load = add_load_component(ctx, instrs, node, k, loc))) + return NULL; + + if (!(store = hlsl_new_store_component(ctx, &block, &var_deref, k, &load->node))) + return NULL; + list_move_tail(instrs, &block.instrs); + } + + if (!(load = hlsl_new_var_load(ctx, var, *loc))) + return NULL; + list_add_tail(instrs, &load->node.entry); + + return &load->node; + } Can we reuse the above branch for this?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/33#note_11243
On 19-10-22 01:54, Zebediah Figura (@zfigura) wrote:
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
+ for (k = 0; k < dst_component_count; ++k) + { + if (!(load = add_load_component(ctx, instrs, node, k, loc))) + return NULL; + + if (!(store = hlsl_new_store_component(ctx, &block, &var_deref, k, &load->node))) + return NULL; + list_move_tail(instrs, &block.instrs); + } + + if (!(load = hlsl_new_var_load(ctx, var, *loc))) + return NULL; + list_add_tail(instrs, &load->node.entry); + + return &load->node; + } Can we reuse the above branch for this?
At first I thought that it would be ugly to get all the cases tangled in the same place, but looking at it again, yes, it is probably better. Will do.
This merge request was closed by Francisco Casas. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/33
Closing MR, because it is superseded by !35. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/33#note_11450
participants (5)
-
Francisco Casas -
Francisco Casas -
Francisco Casas (＠fcasas)
-
Zebediah Figura -
Zebediah Figura (＠zfigura)