Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Support complex implicit casts, complex explicit casts and complex broadcasts. (PART 2/3 v2)

Merged Francisco Casas requested to merge fcasas/vkd3d:complex_broadcast_3 into master
5 unresolved threads

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

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Francisco Casas mentioned in merge request !33 (closed)

    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.
  • Francisco Casas added 6 commits

    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.

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • added 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.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading