Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Fix float modulus for vector arguments.

Merged Nikolay Sivov requested to merge nsivov/vkd3d:float_mod into master
1 unresolved thread

Signed-off-by: Nikolay Sivov nsivov@codeweavers.com

Merge request reports

Merge request pipeline #15442 skipped

Merge request pipeline skipped for d9c984c1

Approved by

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (Sep 22, 2023 8:46pm UTC)

Merge details

  • Changes merged into master with d9c984c1.
  • Deleted the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Nikolay Sivov added 1 commit

    added 1 commit

    • 846297e8 - vkd3d-shader/hlsl: Fix float modulus for vector arguments.

    Compare with previous version

    • Given that hlsl_add_conditional() is also used in lower_int_division() and lower_int_modulus(), which likely have the same problem, shouldn't we rather fix hlsl_add_conditional() instead?

      Also, please add the corresponding test in arithmetic-float-uniform.shader_test too:

      [pixel shader]
      float4 x = {5.0, -42.1, 4.0, 45.0};
      float4 y = {15.0, -5.0, 4.1, 5.0};
      
      float4 main() : sv_target
      {
          return x % y;
      }
      
      [test]
      draw quad
      probe all rgba (5.0, -2.1, 4.0, 0.0) 4
    • Author Developer

      I don't think we should, because it does not mean the same thing. In the future we might want to turn certain if/else blocks to conditional moves, I don't know if that happens on Windows. Regarding this test, it only differs in a way the data is initialized, why is it important?

    • That's just an internal helper for us, it doesn't matter what happens on Windows or will happen in the future.

      WRT the test, the general philosophy is to keep arithmetic-float.shader_test and arithmetic-float-uniform.shader_test synchronized, so that the correctness of the constant propagation pass is checked too. Actually, I got it backward: the test you wrote should go into arithmetic-float-uniform.shader_test, and the test I wrote should go to arithmetic-float.shader_test.

    • Author Developer

      That's just an internal helper for us, it doesn't matter what happens on Windows or will happen in the future.

      I think you misunderstood. What I mean is that explict 'if (x) a=b; else a=c;' might be turned to conditional move with x.xxxx mask, as an optimization. The helper is internal, but I'd rather remove it when it's not useful.

    • Author Developer

      WRT the test, the general philosophy is to keep arithmetic-float.shader_test and arithmetic-float-uniform.shader_test synchronized, so that the correctness of the constant propagation pass is checked too. Actually, I got it backward: the test you wrote should go into arithmetic-float-uniform.shader_test, and the test I wrote should go to arithmetic-float.shader_test.

      Shouldn't it all go to uniform.shader_test then if it's using uniforms?

    • Shouldn't it all go to uniform.shader_test then if it's using uniforms?

      Yeah, the correct form of my test is actually this:

      [pixel shader]
      float4 main() : sv_target
      {
          float4 x = {5.0, -42.1, 4.0, 45.0};
          float4 y = {15.0, -5.0, 4.1, 5.0};
          return x % y;
      }
      
      [test]
      draw quad
      probe all rgba (5.0, -2.1, 4.0, 0.0) 4

      Otherwise it doesn't even compile. And that's supposed to be in arithmetic-float.shader_test, it doesn't use uniform variables.

    • Author Developer

      Ok, tests updated.

    • I think you misunderstood. What I mean is that explict 'if (x) a=b; else a=c;' might be turned to conditional move with x.xxxx mask, as an optimization. The helper is internal, but I'd rather remove it when it's not useful.

      Arguably it's still useful as a convenient shorthand for ternary hlsl_new_expr().

    • Author Developer

      Sure, we can do that. Does it have to be in this MR, or separately? I think it works either way, I wasn't planning on it, but I can amend with int handling changes, together with corresponding tests.

    • My favourite way forward would be, in this MR, to change hlsl_add_conditional() so that it generates a TERNARY instead of an if/else block. In this way all the three sites currently calling it are fixed at once and without (other) changes.

    • Author Developer

      Ok, I'll do that, let's see what happens.

    • Please register or sign in to reply
  • Nikolay Sivov added 1 commit

    added 1 commit

    • ad9a6e74 - vkd3d-shader/hlsl: Fix float modulus for vector arguments.

    Compare with previous version

  • Nikolay Sivov resolved all threads

    resolved all threads

  • Nikolay Sivov added 2 commits

    added 2 commits

    • b8a55f59 - vkd3d-shader/hlsl: Use conditional moves for arithmetic operators instead of branching.
    • e82ad293 - vkd3d-shader/hlsl: Add constant folding for the ternary operator.

    Compare with previous version

  • Author Developer

    Switched all of the operators to use conditional move, updated the tests too.

  • The constant folding is right for MOVC but wrong for TERNARY, since TERNARY does arithmetic comparison.

  • Giovanni Mascellani
  • Nikolay Sivov added 1 commit

    added 1 commit

    • bef51780 - vkd3d-shader/hlsl: Add constant folding for the ternary operator.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard added 73 commits

    added 73 commits

    • bef51780...34b1c0fe - 71 commits from branch wine:master
    • 6d1ba838 - vkd3d-shader/hlsl: Use conditional moves for arithmetic operators instead of branching.
    • d9c984c1 - vkd3d-shader/hlsl: Add constant folding for the ternary operator.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading