Skip to content
Snippets Groups Projects

vkd3d-shader/ir: Refactor VSIR passes.

Merged Giovanni Mascellani requested to merge giomasce/vkd3d:etna into master
3 unresolved threads

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
2233 2233 struct vkd3d_shader_instruction_array *instructions = &parser->instructions;
2234 2234 enum vkd3d_result result = VKD3D_OK;
2235 2235
2236 if (parser->shader_desc.is_dxil)
2237 return result;
2238
2239 if (parser->shader_version.type != VKD3D_SHADER_TYPE_PIXEL)
2236 if (!parser->shader_desc.is_dxil)
  • I have an alternate version of this in !579, using separate vkd3d_shader_normalise_sm4() and vkd3d_shader_normalise_sm6() per your suggestion.

  • Yeah, after some experiments I thought this might have been a better alternative. But I'm not terribly attached to either (and either looks better than the status quo to me). Votes welcome.

  • The fact that we can potentially have passes that should be called on both vkd3d_shader_normalise_sm4() and vkd3d_shader_normalise_sm6(), such as remove_dcl_temps() in 3/3 may be an argument against separating the function, but I don't know if we will end up with enough for the repetition to be a problem.

  • The fact that we can potentially have passes that should be called on both vkd3d_shader_normalise_sm4() and vkd3d_shader_normalise_sm6(), such as remove_dcl_temps() in 3/3 may be an argument against separating the function, but I don't know if we will end up with enough for the repetition to be a problem.

    Yeah, I guess it's still a bit early to commit on that decision. Right now both alternatives look essentially equivalent.

  • Please register or sign in to reply
  • Francisco Casas mentioned in merge request !514 (merged)

    mentioned in merge request !514 (merged)

    • Votes welcome.

      I don't hate either version. I imagine that ultimately we may want to move to a scheme like this though:

          if (program->cf_type == CF_TYPE_STRUCTURED)
              flatten_control_flow_constructs(program);
          [...]

      where "cf_type" would originally be set by the parser. I.e., explicitly checking for properties of the vsir instead of having those be implied by the shader model or bytecode format.

    • I don't hate either version. I imagine that ultimately we may want to move to a scheme like this though:

          if (program->cf_type == CF_TYPE_STRUCTURED)
              flatten_control_flow_constructs(program);
          [...]

      where "cf_type" would originally be set by the parser. I.e., explicitly checking for properties of the vsir instead of having those be implied by the shader model or bytecode format.

      Yes, that's my idea too. Relatedly, as was already discussed, I expect support for different dialects/flags to eventually appear in order to validate language features that do not necessarily hold at every step, but which are still worthwhile to check at every step where they should hold.

    • Please register or sign in to reply
  • Francisco Casas
    Francisco Casas @fcasas started a thread on an outdated change in commit 5636617b
  • 2658 2636 case VKD3DSIH_DCL_TEMPS:
    2659 2637 vsir_validate_dst_count(ctx, instruction, 0);
    2660 2638 vsir_validate_src_count(ctx, instruction, 0);
    2661 if (ctx->dcl_temps_found)
    2662 validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_DUPLICATE_DCL_TEMPS, "Duplicate DCL_TEMPS instruction.");
    • Comment on lines -2661 to -2662

      It is necessary to remove dcl_temps_found? It may still be useful to detect duplicated DCL_TEMPS instructions.

    • In theory yes, but for the moment I mostly see the validator as a device to ease development (by providing a reference on the VSIR syntax and therefore an interface between frontend, passes and backend) rather than a picky language checker for the sake of adhering to a specific standard. Since basically developing passes and backends is much easier using temp_count, I currently see checking specifically that DCL_TEMPS is not duplicate as out of scope.

    • Giovanni Mascellani changed this line in version 2 of the diff

      changed this line in version 2 of the diff

    • On second thought, I noticed that it was easy to keep the duplicate DCL_TEMPS check, so I added it back. The general point is still valid, but in this specific case there is indeed no reason to kill correct code.

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

    • 36774d6a - vkd3d-shader/ir: Remove DCL_TEMPS instructions.

    Compare with previous version

  • Francisco Casas approved this merge request

    approved this merge request

  • added 59 commits

    • 36774d6a...72e2eeaf - 56 commits from branch wine:master
    • cb22b4c2 - vkd3d-shader/ir: Refactor return code checking in vkd3d_shader_normalise().
    • 6e822817 - vkd3d-shader/ir: Run tracing and validation for DXIL code too.
    • c06214ca - vkd3d-shader/ir: Remove DCL_TEMPS instructions.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading