Skip to content
Snippets Groups Projects

vkd3d-shader: Implement RCP opcode for SM4, and rcp() intrinsic.

Merged Petrichor Park requested to merge petrathekat/vkd3d:impl-rcp into master

I'm not quite sure how I should split up this MR into commits; please let me know if I should split the rcp tests from the rcp impl, or squish everything into one big commit.

Merge request reports

Merge request pipeline #28833 skipped

Merge request pipeline skipped for bec4f413

Merged by Henri VerbeetHenri Verbeet 9 months ago (Jul 11, 2024 3:16pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Petrichor Park added 24 commits

    added 24 commits

    • 9c738113...7b4a1fdf - 22 commits from branch wine:master
    • b0b5479c - vkd3d-shader/tpf: Implmenent hlsl RCP opcode for SM4.
    • d64331f7 - vkd3d-shader/hlsl: Implement rcp() intrinsic.

    Compare with previous version

    • This should use VKD3D_SM5_OP_RCP for SM5. For tests, it makes sense to test scalar, vector and matrix types.

    • Is write_sm4_expr not only used to write sm4 bytecode? I note that a few of the cases in there write VKD3D_SM5_* opcodes and I'm not sure why that's allowed. Also, it doesn't look like any of the other cases switch on the version of bytecode written ... I think I might just not understand your question.

    • Everything in tpf.c is used for both SM4 and SM5. If you try to compile something using rcp() on Windows, you'll see a difference, depending on profile you're using.

    • Please register or sign in to reply
  • Also, SM5 supports doubles with a separate instructions and some shader flags.

    • This should use VKD3D_SM5_OP_RCP for SM5. For tests, it makes sense to test scalar, vector and matrix types.

      Eventually, though I don't think it needs to block the patch.

      Also, SM5 supports doubles with a separate instructions and some shader flags.

      Yes. Note that we don't implement doubles anywhere else, so it's fine to leave that as a fixme, but assert(type_is_float()) is wrong.

    • Following the example of HLSL_OP2_DOT, it's OK to just use hlsl_fixme for ints and uints too? I would assume that we would want some kind of separate handling because rcp of an int is invalid, not NYI.

    • Integer division shouldn't reach here, no.

    • This should use VKD3D_SM5_OP_RCP for SM5. For tests, it makes sense to test scalar, vector and matrix types.

      Eventually, though I don't think it needs to block the patch.

      It's a matter of a version check to produce correct output. For doubles sure, this isn't going to be the first place where it doesn't work.

    • So, check if tpf->ctx->profile->major_version == 5, and if so, write VKD3D_SM5_OP1_RCP? It doesn't make sense to me to write sm5 opcodes in a function called write_sm4_expr... and nowhere else in the function is there version checking.

      (Edit: there's version checking elsewhere in the file)

      Edited by Petrichor Park
    • Some debug output shows that the major_version seems to always be 4 and minor_version and {minor,major}_level fields are always 0.

      I guess I don't understand what you're asking me to do.

    • So, check if tpf->ctx->profile->major_version == 5, and if so, write VKD3D_SM5_OP1_RCP? It doesn't make sense to me to write sm5 opcodes in a function called write_sm4_expr... and nowhere else in the function is there version checking.

      "sm4" usually means models 4-5, as sm1 means 1-3. "tpf" and "d3dbc" are synonymous with these respectively. Neither naming scheme is perfect.

      Some debug output shows that the major_version seems to always be 4 and minor_version and {minor,major}_level fields are always 0.

      That's because the tests only check with 4.0. It's possible to compile with 5.0. E.g. 'vkd3d-compiler test.hlsl -o test.hlsl.o -p ps_5_0'.

    • Please register or sign in to reply
  • +            for (int i = 0; i < 4; i++)

    "unsigned int"

    •            one.u[i].f = 1.f;

    "1.0f"

  • Petrichor Park added 19 commits

    added 19 commits

    Compare with previous version

  • Petrichor Park added 20 commits

    added 20 commits

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

    • Subject: [PATCH 1/2] vkd3d-shader/tpf: Implmenent hlsl RCP opcode.

      Typo in the commit message.

      I think the test should come first in this series.

      +        case HLSL_OP1_RCP:
      +            switch (dst_type->base_type)
      +            {
      +                case HLSL_TYPE_FLOAT:
       ...
      +                default:
      +                    hlsl_fixme(tpf->ctx, &expr->node.loc, "SM4 %s rcp expression.", dst_type_string->buffer);
      +            }
      +            break;

      Does rcp()/HLSL_OP1_RCP actually support non-float inputs?

      +float4 main() : sv_target
      +{
      +»······return rcp(f);
      +}

      We generally don't use tabs for indentation.

    • Does rcp()/HLSL_OP1_RCP actually support non-float inputs?

      I don't think it does. Zev mentioned to me in the company chat that execution shouldn't reach this point if you try to take the rcp of an int, though. That's probably a good thing to add to the tests.

    • Actually according to the shader playground, it does accept non-float inputs. The Microsoft docs (https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/rcp) say that it only accepts floats, so an int must get projected into a float when you pass it as an argument. This also seems to be the behavior of our implementation.

      Do we have support for bytecode tests? If we do, I can add a test to make sure it won't RCP an int.

    • For most of the functions we add explicit cast to float. Since it's the only place where OP_RCP is created, you can assume it's not going to reach bytecode output with any other argument type.

    • Please register or sign in to reply
  • Petrichor Park added 3 commits

    added 3 commits

    • 183916c5 - vkd3d-shader/hlsl: Implement tests for rcp() intrinsic.
    • b195bc0f - vkd3d-shader/tpf: Implmenent hlsl RCP opcode.
    • 5500b46e - vkd3d-shader/hlsl: Implement rcp() intrinisic.

    Compare with previous version

5043 5043 }
5044 5044 break;
5045 5045
5046 case HLSL_OP1_RCP:
5047 switch (dst_type->base_type)
5048 {
5049 case HLSL_TYPE_FLOAT:
5050 /* SM5 comes with a RCP opcode */
5051 if (tpf->ctx->profile->major_version >= 5)
5052 write_sm4_unary_op(tpf, VKD3D_SM5_OP_RCP, &expr->node, arg1, 0);
  • Personally I don't need to have it fixed here, but for next time I think it makes sense to order commits in this way:

    • first the tests (which you did);
    • then the frontend implementation;
    • then the backend implementation.

    You swapped the last two point, which leaves some code practically dead for a short window. Basically the idea is that your implementation moves in the same direction as the flow of data.

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • I just noticed tests are failing because the SM6 are marked as todo. You should probably use todo(sm<6).

  • Petrichor Park added 210 commits

    added 210 commits

    • 5500b46e...46a1b66d - 207 commits from branch wine:master
    • 88ac0b97 - vkd3d-shader/hlsl: Implement tests for rcp() intrinsic.
    • c2da340a - vkd3d-shader/hlsl: Implement rcp() intrinisic.
    • 3b975018 - vkd3d-shader/tpf: Implmenent hlsl RCP opcode.

    Compare with previous version

  • Whoops, I did not mean to submit this to the main MR; I just wanted it on Gitlab so I could check the CI results (on my own fork). Let's see if it works anyways.

  • Petrichor Park added 3 commits

    added 3 commits

    • 98221247 - vkd3d-shader/hlsl: Implement tests for rcp() intrinsic.
    • 2b6c0eeb - vkd3d-shader/hlsl: Implement rcp() intrinisic.
    • 6c854cf5 - vkd3d-shader/tpf: Implmenent hlsl RCP opcode.

    Compare with previous version

  • Petrichor Park added 7 commits

    added 7 commits

    • 6c854cf5...ccb6150a - 5 commits from branch wine:master
    • db2f9112 - vkd3d-shader/hlsl: Implement tests for rcp() intrinsic.
    • fcb642ea - vkd3d-shader/hlsl: Implement rcp() intrinisic.

    Compare with previous version

  • Petrichor Park added 3 commits

    added 3 commits

    • 87b45a6c - vkd3d-shader/hlsl: Implement tests for rcp() intrinsic.
    • e92f0d62 - vkd3d-shader/hlsl: Implement rcp() intrinisic.
    • 6acb2d55 - vkd3d-shader/tpf: Implmenent hlsl RCP opcode.

    Compare with previous version

  • Petrichor Park added 53 commits

    added 53 commits

    • 6acb2d55...67c690aa - 50 commits from branch wine:master
    • ddaa5a76 - vkd3d-shader/hlsl: Implement tests for rcp() intrinsic.
    • 1c725da5 - vkd3d-shader/hlsl: Implement rcp() intrinisic.
    • a0e79a62 - vkd3d-shader/tpf: Implmenent hlsl RCP opcode.

    Compare with previous version

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