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/
Merge request reports
Activity
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.)
Francisco Casas replied on the mailing list:
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/838e575fdde5b15cfde785250d849dfbca4458f4 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.
396 for (k = 0; k < dst_component_count; ++k) 397 { 398 if (!(load = add_load_component(ctx, instrs, node, k, loc))) 399 return NULL; 400 401 if (!(store = hlsl_new_store_component(ctx, &block, &var_deref, k, &load->node))) 402 return NULL; 403 list_move_tail(instrs, &block.instrs); 404 } 405 406 if (!(load = hlsl_new_var_load(ctx, var, *loc))) 407 return NULL; 408 list_add_tail(instrs, &load->node.entry); 409 410 return &load->node; 411 } Francisco Casas replied on the mailing list:
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.
Zebediah Figura replied on the mailing list:
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/838e575fdde5b15cfde785250d849dfbca4458f4 > > 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.
mentioned in merge request !35 (merged)
Closing MR, because it is superseded by !35 (merged).