Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Add support for ddx(), ddy() intrinsics.

Merged Ethan Lee requested to merge flibitijibibo/vkd3d:dsxdsy into master

SPIR-V already handled DSX/DSY, so only D3DBC/TPF needed new case blocks.

You'll notice that there's no test for this one - in addition to being a pretty straightforward translation for all possible formats, this feature uses the render target width/height and I wasn't sure if there was a good way to ensure that the test would always make sense.

Instead, I did the test manually, and it's what you'd expect (EDIT: Previously the test used a uniform which always optimized to 0, new test uses VPOS instead):

HLSL:

float4 main(float4 pos : sv_position) : sv_target
{
    float4 x = ddx(pos.x);
    float4 y = ddy(pos.y);
    return x + y;
}

D3DBC:

ps_3_0
dcl_position0 vPos
mov r0.xyzw, vPos.xyzw
mov r1.x, r0.x
dsx r1.x, r1.x
mov r0.x, r0.yxxx
dsy r0.x, r0.x
mov r1.xyzw, r1.x
mov r0.xyzw, r0.x
add r0.xyzw, r1.xyzw, r0.xyzw
mov oC0.xyzw, r0.xyzw

DXBC-TPF:

ps_4_0
dcl_input_ps_siv linear v0.xyzw, position
dcl_output o0.xyzw
dcl_temps 2
mov r0.xyzw, v0.xyzw
mov r1.x, r0.x
dsx r1.x, r1.x
mov r0.x, r0.yxxx
dsy r0.x, r0.x
mov r1.xyzw, r1.x
mov r0.xyzw, r0.x
add r0.xyzw, r1.xyzw, r0.xyzw
mov o0.xyzw, r0.xyzw
ret

Fixes https://bugs.winehq.org/show_bug.cgi?id=54827

Edited by Ethan Lee

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
  • Ethan Lee changed the description

    changed the description

    • Resolved by Ethan Lee

      I don't know if that's an optimization effect, but code in your example produces zero output unconditionally:

      ps_4_0
      dcl_output o0.xyzw
      mov o0.xyzw, l(0,0,0,0)
      ret 

      I don't know how this works exactly, but maybe constant arguments produce 0 derivatives, and using expression that depends on certain semantics generates actual instructions.

  • Ethan Lee changed the description

    changed the description

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Ethan Lee added 5 commits

    added 5 commits

    Compare with previous version

  • Francisco Casas approved this merge request

    approved this merge request

  • Ethan Lee added 1 commit

    added 1 commit

    • 5917fdf1 - tests: Add test for ddx(), ddy() intrinsics

    Compare with previous version

    • Added a test called ddxddy, which replicates the OP's test. It's sort of an unremarkable test, because I'm not too familiar with this feature and am not sure what a better test would look like.

      We'd probably want to use some trivial but non-linear function like e.g. a quadratic (or perhaps e.g. a sine or cosine) over sv_position as input to ddx()/ddy(), and then probe the output in some number (e.g. 16) of places.

    • Yeah, there is some room to make the test more specific. One other way is to output float4(dsx, dsy, 0.0, 0.0) instead of the sum. Also, given the peculiar way derivatives are computed in shading languages, it makes sense to test both even and odd pixel coordinates.

      But at least exercising the code path is already much better than nothing, and good enough, for me, to approve (can't speak for Henri, though! :-) ).

    • Author Contributor

      Latest push splits the results of ddx/ddy to be separate. In trying to come up with a test that produces more interesting results it seems like it just becomes a test for other functions like sin/cos, since the result with vpos is always going to be 1... this is what I get for not paying attention in math class :P

    • Mostly for fun, here is a somewhat more fancy test:

      [pixel shader]
      float4 main(float4 pos : sv_position) : sv_target
      {
          pos /= 10.0;
          float nonlinear = pos.x * pos.y - pos.x * (pos.x + 0.5);
          return float4(nonlinear, ddx(nonlinear), ddy(nonlinear), 0.0);
      }
      
      [test]
      draw quad
      probe (10, 10) rgba (-0.524999976, -0.164999843, 0.104999900, 0.0) 8
      probe (11, 10) rgba (-0.689999819, -0.164999843, 0.104999900, 0.0) 8
      probe (10, 11) rgba (-0.420000076, -0.164999843, 0.104999900, 0.0) 8
      probe (11, 11) rgba (-0.574999928, -0.164999843, 0.104999900, 0.0) 8
      probe (12, 10) rgba (-0.874999881, -0.205000162, 0.124999881, 0.0) 8
      probe (150, 150) rgba (-7.52500916, -1.56500244, 1.50500488, 0.0) 8

      An interesting feature is that the partial derivatives are constant across the whole 2x2 tile for all the three drivers I tested (WARP on Windows, RADV on Linux and NVIDIA proprietary on Linux), a non-trivial fact that I would not have given for granted before testing.

      I also did some tests with trigonometric functions, but NVIDIA's implementation is so wonky that it's nearly impossible to avoid diverging from the other drivers.

    • Author Contributor

      Oddly enough this doesn't seem to pass on RADV with a 7900XTX, but just barely:

      shader_runner:1197: Running tests from a Unix build
      shader_runner:1199: Compiling shaders with vkd3d-shader and executing with Vulkan
      shader_runner:601: Section [test], line 19: Test failed: Got {-7.52500343e+00, -1.56500721e+00, 1.50500488e+00, 0.00000000e+00}, expected {-7.52500916e+00, -1.56500244e+00, 1.50500488e+00, 0.00000000e+00} at (150, 150).
      shader_runner:1202: Compiling shaders with vkd3d-shader and executing with vkd3d
      shader_runner:612: Driver name: radv, driver info: Mesa 23.2.0-devel.
      shader_runner:601: Section [test], line 19: Test failed: Got {-7.52500343e+00, -1.56500721e+00, 1.50500488e+00, 0.00000000e+00}, expected {-7.52500916e+00, -1.56500244e+00, 1.50500488e+00, 0.00000000e+00} at (150, 150).
      shader_runner: 149 tests executed (2 failures, 0 skipped, 0 todo, 0 bugs).
      FAIL tests/ddxddy.shader_test (exit status: 1)
    • I would have hoped that 8 ULPs were enough for having a common idea of the first derivatives of a quadratic polynomial, but it seems video card producers decided differently. I don't think there is much we can do about, except for raising our tolerance (which is the number at the end of the probe command).

    • Author Contributor

      Messed with the tolerance values a bit, all of the tests are actually okay at 8 except for the last one, which could go no lower than 40. That seems... really high for one pixel? If it isn't, I can push this test with that one tolerance value set higher than the others.

    • Strange, not sure why it goes that astray, but I wouldn't sweat too much over that. Pushing 40 is fine.

    • Author Contributor

      Understood, test has been pushed!

    • 40 isn't that bad, no. Our trigonometry tests do far worse.

      I'm not particularly surprised that derivatives aren't very accurate anyway.

    • Please register or sign in to reply
  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Ethan Lee added 1 commit

    added 1 commit

    • 28e4c350 - tests: Add test for ddx(), ddy() intrinsics

    Compare with previous version

  • Ethan Lee added 9 commits

    added 9 commits

    • 28e4c350...af4bb037 - 7 commits from branch wine:master
    • d032ddb3 - vkd3d-shader/hlsl: Add support for ddx(), ddy() intrinsics.
    • 43209e08 - tests: Add test for ddx(), ddy() intrinsics

    Compare with previous version

  • I wouldn't want to dump a pile of work on someone, but note that HLSL has ddx_coarse()/ddy_coarse() and ddx_fine()/ddy_fine() variants of ddx()/ddy() as well. It seems straightforward enough to implement those as well as a follow-up to this MR.

  • In any case, this seems fine. It would be nice if Giovanni's test could be integrated before this gets committed, but adding that as a follow-up is fine too. I do think we want that test.

  • Henri Verbeet approved this merge request

    approved this merge request

  • Ethan Lee added 1 commit

    added 1 commit

    • abe1c35b - tests: Add tests for ddx(), ddy() intrinsics.

    Compare with previous version

  • Ethan Lee resolved all threads

    resolved all threads

  • added 2 commits

    • 138c32ce - vkd3d-shader/hlsl: Add support for ddx(), ddy() intrinsics.
    • d3876e49 - tests: Add tests for ddx(), ddy() intrinsics.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply
Loading