Skip to content
Snippets Groups Projects

vkd3d-shader/dxil: Implement DX instruction StoreOutput.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:dxil_instructions into master

Merge request reports

Checking pipeline status.

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (Aug 30, 2023 9:19pm 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
  • Conor McCarthy added 3 commits

    added 3 commits

    • d623fea2 - vkd3d-shader/dxil: Implement the DXIL CALL instruction.
    • c49fb922 - vkd3d-shader/dxil: Declare shader outputs.
    • a3646828 - vkd3d-shader/dxil: Implement DX instruction StoreOutput.

    Compare with previous version

  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • It might be that this ship has already sailed, since this is probably something preceding this MR, but I feel a bit awkward about how errors are silently ignored in a number of code paths (see for example sm6_parser_emit_dx_store_output(), but it's not the only place). I understand that we don't want to be overly zealous with rejecting invalid shaders, because it might be that even if our translation isn't perfect the translated shader is still good enough so that, say, a certain game is playable.

    However, at least from the point of view of vkd3d-shader (which doesn't know how that shader is going to be used), this doesn't really feel correct. Maybe the average game doesn't really care if a shader is broken, since the bad case scenario is that the player just decides that the game isn't fun and proceeds doing something else. But other applications might be more demanding. For example, if you allow me a rather extreme example, a medical application might be much less failure-tolerant, to the point of preferring declaring a malfunction rather than plotting wrong medical data. I'm not aware of any such usage, currently, for vkd3d-shader, but I consider this an example to infer that the decision of still using a shader even if it's known to be incorrectly translated should not belong to vkd3d-shader. It makes sense for vkd3d-shader to still report an incorrectly translated shader as a best guess, but the error code should declare what's happening, so that the client can choose meaningfully.

    What is the feeling about this issue?

  • Author Developer

    Failure of sm6_value_get_constant_uint() is not handled for column_index, and sm6_parser_function_init() returns VKD3D_OK on paramater allocation failure. I intend to fix these. Are there other issues?

  • Why not always returning the error? What is the advantage of doing some more parsing, if the only information we're going to return is just a generic failure error, without additional diagnostics?

    Well, we should be providing the additional diagnostics. But generally, for much the same reasons that e.g. the HLSL compiler doesn't abort at the first sign of trouble. There may be multiple unrelated issues in the same shader, and knowing about subsequent issues sooner rather than later is generally helpful. At the very least because it might give someone interested in working on an application a better sense of the amount of effort required to make it work.

    It might be that this ship has already sailed, since this is probably something preceding this MR, but I feel a bit awkward about how errors are silently ignored in a number of code paths (see for example sm6_parser_emit_dx_store_output(), but it's not the only place). I understand that we don't want to be overly zealous with rejecting invalid shaders, because it might be that even if our translation isn't perfect the translated shader is still good enough so that, say, a certain game is playable.

    No, I think you're right about that; the default behaviour should be to reject invalid shaders. We may want to be able to relax that in some cases, but we could introduce compile options for that, perhaps enabled by VKD3D_CONFIG flags on the vkd3d side.

  • Conor McCarthy added 3 commits

    added 3 commits

    • 70bd0bf1 - vkd3d-shader/dxil: Implement the DXIL CALL instruction.
    • daad554c - vkd3d-shader/dxil: Declare shader outputs.
    • ddc7e724 - vkd3d-shader/dxil: Implement DX instruction StoreOutput.

    Compare with previous version

  • Conor McCarthy added 3 commits

    added 3 commits

    • c89227da - vkd3d-shader/dxil: Implement the DXIL CALL instruction.
    • 3eaa5f6c - vkd3d-shader/dxil: Declare shader outputs.
    • cf3699e7 - vkd3d-shader/dxil: Implement DX instruction StoreOutput.

    Compare with previous version

  • Giovanni Mascellani
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit cf3699e7
  • 2246 2471 FIXME("Unhandled dxil instruction %u.\n", record->code);
    2247 2472 return VKD3D_ERROR_INVALID_SHADER;
    2248 2473 }
    2474 /* Allocation failure. */
    • That's not necessarily the case, is it? Any call to vkd3d_shader_parser_error(), such as in dxil_record_validate_operand_min_count(), could trigger this failure.

      More in general, I think we there is some confusion between whether to use the parser failed bit vs using the return value. We could use the same convention as the HLSL compiler: failed is set as soon as an error is found, and it should always set via vkd3d_shader_parser_error() so that a diagnostic is always available; an error return value should only be used when an error was found and parsing must halt immediately.

      In this scheme you should never look at failed to decide what to return, because you don't know if failed was set because of an error that still allows parsing or not. You have to genuinely propagate the return code at each level.

      It is useful to have memory allocation functions that also set the failed bit, like the HLSL compiler.

    • Conor McCarthy changed this line in version 6 of the diff

      changed this line in version 6 of the diff

    • Please register or sign in to reply
  • Conor McCarthy added 4 commits

    added 4 commits

    • 03dca249 - vkd3d-shader/dxil: Validate the function block count.
    • 988d4409 - vkd3d-shader/dxil: Implement the DXIL CALL instruction.
    • 836625f8 - vkd3d-shader/dxil: Declare shader outputs.
    • efdeab5f - vkd3d-shader/dxil: Implement DX instruction StoreOutput.

    Compare with previous version

  • Conor McCarthy added 3 commits

    added 3 commits

    • 85736320 - vkd3d-shader/dxil: Implement the DXIL CALL instruction.
    • 4254a357 - vkd3d-shader/dxil: Declare shader outputs.
    • b430cd5c - vkd3d-shader/dxil: Implement DX instruction StoreOutput.

    Compare with previous version

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