Skip to content
Snippets Groups Projects

vkd3d-shader/spirv: Implement MAD in two operations if flagged as precise.

Closed Conor McCarthy requested to merge cmccarthy/vkd3d:precise_mad into master
2 unresolved threads

This would eliminate the todo for the precise mad() test in !718 (merged). Maybe we need test results on nvidia and intel to decide if we actually want this.

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
  • Conor McCarthy added 25 commits

    added 25 commits

    • 0ceeec95...164608a0 - 24 commits from branch wine:master
    • 4860d22c - vkd3d-shader/spirv: Implement MAD in two operations if flagged as precise.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • ec46b7a6 - vkd3d-shader/spirv: Implement MAD in two operations if flagged as precise.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • I suspect this MR will need a rebase in any case.

    +        /* The HLSL docs state: "If components of a mad instruction are tagged as precise, the
    +         * hardware must execute a mad instruction or the exact equivalent, and it cannot split
    +         * it into a multiply followed by an add."
    +         * But DXIL.rst states the opposite: "Floating point multiply & add. This operation is
    +         * not fused for "precise" operations."
    +         * Windows drivers seem to conform with the latter, for SM 4-5 and SM 6. */

    The comment seems a bit unfortunate/ambiguous. The "HLSL docs" here refer to the "Shader Model 5 Assembly" documentation. (As opposed to e.g. referring to the translation of the HLSL mad() intrinsic to d3dbc/tpf.)

    Do we want to handle this in the SPIR-V backend? It seems like something vsir_program_normalise() could take care of, possibly as part of the current vsir_program_lower_texkills() pass.

  • Conor McCarthy added 84 commits

    added 84 commits

    • ec46b7a6...9c0d04c8 - 83 commits from branch wine:master
    • 251d8521 - vkd3d-shader/spirv: Implement MAD in two operations if flagged as precise.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • 092f7958 - vkd3d-shader/ir: Implement MAD in two operations if flagged as precise.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • 4f98ef63 - vkd3d-shader/ir: Implement MAD in two operations if flagged as precise.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • 86b55c1c - vkd3d-shader/ir: Implement MAD in two operations if flagged as precise.

    Compare with previous version

  • Giovanni Mascellani unapproved this merge request

    unapproved this merge request

    • I don't think the flattener should be "hijacked" for this pass, it is already complicated enough. It's probably appropriate to have a single pass to collect all the little local operations like this one, instead of iterating over the whole instruction array every time, but it should be another one. As Henri notices, the same pass might do this and lowering texkills (and probably removing DCL_TEMPS too).

    • Author Developer

      We have no other passes which may need to insert instructions. It's a compromise between adding yet another instruction copying pass, versus favouring performance even if it's a little messy. If the former is the priority I'll add a pass for all the small operations.

    • Author Developer

      I added it to lower_texkills(). Moving instructions for each occurrence takes quadratic time, so we could see pathological cases. For example, Cyberpunk has a colossal compute shader with over 40,000 SSA values, so a shader of similar size which uses precise MAD could occur. If necessary we could convert lower_texkills_and_precise_mad() to copy instead of insert.

    • Yeah, I still don't like the way that pass is structured and I think it would be a better idea to rewrite it in terms of a full program copy like many other passes are structured. But that's an independent issue that might be addressed at some point: in terms of software design I think it's better to lump all these smaller changes to the shader in a single pass.

      However, what do you think of refactoring your MR in a manner similar to this (last three commits)?

    • Sorry for the double comment, but let me add another couple of remarks:

      • At some point it might happen that the instruction written by one of the helpers called by vsir_program_lower_instruction() should itself be processed again. To handle this we could have the switch cases return VKD3D_OK when they did something and VKD3D_FALSE when they touched nothing, so that the loop in vsir_program_lower_instruction() can decide whether to increment the position or not.
      • Both the TEXKILL and the MAD helpers currently use a TEMP for storing the intermediate values. This should probably become a SSA at some point; even for SM4 there shouldn't be problem for passes to introduce SSAs, provided that they are written only once and the definition point dominates the usage points. The SPIR-V backend should handle them just fine. This has the advantage of not requiring to pass a pointer to all the helpers to coordinate the TEMP index, and I guess in general the less we use TEMPs the better.
    • Author Developer

      Your branch looks good to me. Would you like to submit a new MR with those three commits, while I close mine? A couple of comments: there's a typo in the commit message for DCL_TEMPS, and I would rename vsir_program_lower_mad to vsir_program_lower_precise_mad for accuracy.

      To handle this we could have the switch cases return VKD3D_OK when they did something and VKD3D_FALSE when they touched nothing

      That makes sense if the need arises.

      This should probably become a SSA at some point

      Yes, and the coordinates and texels which the DXIL parser emits should probably be changed at the same time, though we would need a COMPOSITE_CONSTRUCT instruction to make those SSA. Maybe after the release.

    • Ok, submitted at !779 (merged).

    • Please register or sign in to reply
  • Conor McCarthy added 1 commit

    added 1 commit

    • a04cf97b - vkd3d-shader/ir: Implement MAD in two operations if flagged as precise.

    Compare with previous version

  • Yeah, I still don't like the way that pass is structured and I think it would be a better idea to rewrite it in terms of a full program copy like many other passes are structured.

    I broadly disagree. TEXKILL isn't that common, and all this pre-emptive copying that a number of these passes do can't possibly be cheaper than an efficient implementation of shader_instruction_array_insert_at() would be.

    The existing implementation of shader_instruction_array_insert_at() certainly isn't optimal, but that's an issue that can be addressed there; it's not a problem with the passes using it.

    But that's an independent issue that might be addressed at some point: in terms of software design I think it's better to lump all these smaller changes to the shader in a single pass.

    However, what do you think of refactoring your MR in a manner similar to this (last three commits)?

    I didn't review those commits in detail, but in terms of the structure, that's pretty much exactly what I had in mind.

  • Giovanni Mascellani mentioned in merge request !779 (merged)

    mentioned in merge request !779 (merged)

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Giovanni Mascellani unapproved this merge request

    unapproved this merge request

  • closed

Please register or sign in to reply
Loading