vkd3d-shader/hlsl: Support complex implicit casts, complex explicit casts and complex broadcasts. (PART 2/3 v2)
Second version of !33 (closed), taking Zeb's suggestions.
The refactoring allowed to encompass all remaining features of the patch series in these 4 patches.
The remaining patches (PART 3/3) just contain additional tests: https://gitlab.winehq.org/fcasas/vkd3d/-/commits/complex_broadcasts_4/
Merge request reports
Activity
mentioned in merge request !33 (closed)
Zebediah Figura replied on the mailing list:
On 10/19/22 22:36, Francisco Casas wrote: > In the following patches, compatible_data_types() and > implicit_compatible_data_types() are improved so that: > * Casts that should be allowed actually reach add_cast(). > * Casts that shouldn't be allowed because of special conditions (besides > component count), e.g. matrix to matrix, don't reach add_cast(). Not an issue with these patches (nor do I think it should change anything about how they're implemented), but for what it's worth, we probably should let even the invalid casts "reach" add_cast(). Or, at least, they shouldn't abort compilation; maybe we can achieve that in some other way. This is a more general problem we have—we should try to avoid aborting compilation where possible, so that multiple errors can be reported in a single pass. > + matrix_cast = !broadcast && dst_comp_count != src_comp_count > + && src_type->type == HLSL_CLASS_MATRIX && dst_type->type == HLSL_CLASS_MATRIX; > + assert(src_comp_count >= dst_comp_count || broadcast); > + assert(!matrix_cast || dst_type->dimx <= src_type->dimx); > + assert(!matrix_cast || dst_type->dimy <= src_type->dimy); Nitpicking, I think this is a bit easier to read as if (matrix_cast) { assert(dst_type->dimx <= src_type->dimx); assert(dst_type->dimy <= src_type->dimy); }
Zebediah Figura replied on the mailing list:
On 10/19/22 22:36, Francisco Casas wrote: > +static bool compatible_data_types(struct hlsl_ctx *ctx, struct hlsl_type *src, struct hlsl_type *dst) > +{ > + if (src->type <= HLSL_CLASS_LAST_NUMERIC && src->dimx == 1 && dst->dimy == 1 && type_contains_only_numerics(dst)) > + return true; That looks like a copy-paste error; it should be "src->dimy == 1"? Kinda surprised that didn't cause any test failures...
Zebediah Figura replied on the mailing list:
On 10/19/22 22:36, Francisco Casas wrote: > - if ((src->type == HLSL_CLASS_ARRAY && dst->type <= HLSL_CLASS_LAST_NUMERIC) > - || (src->type <= HLSL_CLASS_LAST_NUMERIC && dst->type == HLSL_CLASS_ARRAY)) > - { > - /* e.g. float4[3] to float4 is allowed */ > - if (src->type == HLSL_CLASS_ARRAY && hlsl_types_are_equal(src->e.array.type, dst)) > - return true; > - if (hlsl_type_component_count(src) == hlsl_type_component_count(dst)) > - return true; > - return false; > - } This patch removes the "broken" cases I mentioned, which is great, except that it's different from the main and stated purpose of the patch. Ideally that's something we should pull into a separate patch (earlier, or maybe later, whichever makes more sense). We also would ideally have tests for those illegal casts.
The commit message for d51c8690 has a newline in the subject, which is probably better avoided. If you are concerned about overlong lines, notice that you can save a few strokes rewriting as "Support implicit casts between component-wise equal types".
Francisco Casas replied on the mailing list:
On 20-10-22 02:04, Zebediah Figura wrote: > On 10/19/22 22:36, Francisco Casas wrote: >> - if ((src->type == HLSL_CLASS_ARRAY && dst->type <= >> HLSL_CLASS_LAST_NUMERIC) >> - || (src->type <= HLSL_CLASS_LAST_NUMERIC && dst->type == >> HLSL_CLASS_ARRAY)) >> - { >> - /* e.g. float4[3] to float4 is allowed */ >> - if (src->type == HLSL_CLASS_ARRAY && >> hlsl_types_are_equal(src->e.array.type, dst)) >> - return true; >> - if (hlsl_type_component_count(src) == >> hlsl_type_component_count(dst)) >> - return true; >> - return false; >> - } > > This patch removes the "broken" cases I mentioned, which is great, > except that it's different from the main and stated purpose of the > patch. Ideally that's something we should pull into a separate patch > (earlier, or maybe later, whichever makes more sense). We also would > ideally have tests for those illegal casts. > > Okay, I am doing that in a separate patch earlier. Also I am moving the additional implicit cast tests forward and adding these cases specifically.
Francisco Casas replied on the mailing list:
On 20-10-22 02:04, Zebediah Figura wrote: > On 10/19/22 22:36, Francisco Casas wrote: >> +static bool compatible_data_types(struct hlsl_ctx *ctx, struct >> hlsl_type *src, struct hlsl_type *dst) >> +{ >> + if (src->type <= HLSL_CLASS_LAST_NUMERIC && src->dimx == 1 && >> dst->dimy == 1 && type_contains_only_numerics(dst)) >> + return true; > > That looks like a copy-paste error; it should be "src->dimy == 1"? Kinda > surprised that didn't cause any test failures... > > It indeed was! And after I fixed it, some of the tests in the following patches, specifically [pixel shader fail]s involving matrices, were not passing. So this was one of the weird cases where two bugs even each other out, *sigh*. I am changing compatible_data_types() in the next version to ensure that these shaders indeed fail.
added 6 commits
- 88115062 - vkd3d-shader/hlsl: Rename "t1" and "t2" arguments as "src" and "dst".
- e73cdd0b - tests: Add additional tests for implicit casts that should fail.
- 14fabf32 - vkd3d-shader/hlsl: Handle complex types in add_cast().
- bc3f103b - vkd3d-shader/hlsl: Remove incorrect criteria for accepting implicit casts.
- 3c2be5f3 - vkd3d-shader/hlsl: Support implicit casts between component-wise equal types.
- f28d274e - vkd3d-shader/hlsl: Support explicit cast between component-wise compatible types.
Toggle commit listadded 7 commits
-
963ea98a - 1 commit from branch
wine:master
- 0a345a2b - vkd3d-shader/hlsl: Rename "t1" and "t2" arguments as "src" and "dst".
- fb751b48 - tests: Add additional tests for implicit casts that should fail.
- d93ce289 - vkd3d-shader/hlsl: Handle complex types in add_cast().
- 1c778116 - vkd3d-shader/hlsl: Remove incorrect criteria for accepting implicit casts.
- d21fd584 - vkd3d-shader/hlsl: Support implicit casts between component-wise equal types.
- 5af7316a - vkd3d-shader/hlsl: Support explicit cast between component-wise compatible types.
Toggle commit list-
963ea98a - 1 commit from branch