Skip to content
Snippets Groups Projects

vkd3d-shader/spirv: Merge all shader IR fork and join phases into a single phase.

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

Normalise the incoming vkd3d_shader_instruction IR to the shader model 6 pattern where only one patch constant function is emitted. This allows generation of a single patch constant function in SPIR-V.

Edited by Conor McCarthy

Merge request reports

Merge request pipeline #9260 skipped

Merge request pipeline skipped for c61f9f47

Merged by Alexandre JulliardAlexandre Julliard 2 years ago (Apr 3, 2023 8:08pm 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
  • I found a rare case where an input is not declared in the control point phase but is declared and used in the fork phase. I'll be changing the I/O normalisation patch.

  • Conor McCarthy added 2 commits

    added 2 commits

    • 090c8c75 - vkd3d-shader/spirv: Normalise signatures and input/output registers to the Shader Model 6 pattern.
    • 5c42a606 - vkd3d-shader/trace: Trace the normalised instruction array after tracing the input.

    Compare with previous version

  • I didn't review this yet, but this change did catch my eye:

    diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h
    index 86ffb9d5..3b106e68 100644
    --- a/include/vkd3d_shader.h
    +++ b/include/vkd3d_shader.h
    @@ -1444,6 +1444,11 @@ struct vkd3d_shader_signature_element
         unsigned int used_mask;
         /** Minimum interpolation precision. */
         enum vkd3d_shader_minimum_precision min_precision;
    +    /**
    +     * Register count. A value of 0 or 1 indicates \a register_index is the
    +     * only register. Values greater than 4 are invalid. \since 1.7
    +     */
    +    unsigned int register_count;
     };
     
     /**

    You can't do that; the vkd3d_shader_signature_element structure is part of the public API.

    Without having looked much at the contents, I notice patch 1/4 and 3/4 are quite large; if those could be split that would be much appreciated.

  • I haven't looked at really any of these patches (as Henri mentioned, they're awfully large), but it seems surprising that we're doing this normalization in spirv.c, and not in the dxbc/sm4 frontend.

  • If we don't add a register count to vkd3d_shader_signature_element then API users can't specify SM 6 signatures with it, because they contain a register count ('row count' in the docs but it amounts to the same).

    Patch 1 contains only what's required to resolve multiple fork/join phases into one. I see no intermediate changes which can be made.

    I can probably split off a few small changes from patch 3: merging split I/O registers, separating the relative address from the register index for register relative indexing, inserting the control point count or id in reg.idx where it's missing, and creating a partial default control point phase in the IR. For the remainder, the problem is once the signatures are normalised then all the changes are necessary for it to work.

    It may be feasible to normalise only input signatures, then add outputs later, or vice-versa, bearing in mind sometimes the patch constants are inputs.

    The location for the normalisation is debatable. Henri and I agreed spirv.c is probably the best place because these changes are intended to make emitting SPIR-V easier. It can also be argued that since the SM 6 parser naturally emits in this form, except for one or two fixups, it's not all that related to SPIR-V.

  • In patch 1 I can probably split the removal of the phase array from the other changes.

  • If we don't add a register count to vkd3d_shader_signature_element then API users can't specify SM 6 signatures with it, because they contain a register count ('row count' in the docs but it amounts to the same).

    Sure, it probably makes sense for there to be some way to return complete information for sm6 input signatures (though note that e.g. D3D12_SIGNATURE_PARAMETER_DESC doesn't appear to include this either); we just can't do it by changing the existing vkd3d_shader_signature_element structure because that breaks the ABI. Perhaps most notably because the vkd3d_shader_signature structure contains an array of those.

    The location for the normalisation is debatable. Henri and I agreed spirv.c is probably the best place because these changes are intended to make emitting SPIR-V easier. It can also be argued that since the SM 6 parser naturally emits in this form, except for one or two fixups, it's not all that related to SPIR-V.

    Right, we may change our mind about this at some point in the future. For the moment though, one of the main considerations is that we'd like to preserve the original structure long enough for the d3d-asm target to be able to output it.

  • The location for the normalisation is debatable. Henri and I agreed spirv.c is probably the best place because these changes are intended to make emitting SPIR-V easier. It can also be argued that since the SM 6 parser naturally emits in this form, except for one or two fixups, it's not all that related to SPIR-V.

    Right, we may change our mind about this at some point in the future. For the moment though, one of the main considerations is that we'd like to preserve the original structure long enough for the d3d-asm target to be able to output it.

    I feel like trying to accommodate the assembler (and disassembler) in vkd3d_shader_instruction is going to be a losing proposition at some point, but in this case it at least doesn't seem right that it's in spirv.c; perhaps it could live in some "neutral" source file?

  • Looks like it could be placed in its own source file pretty neatly.

    I'll remind everyone, given the magnitude of the changes in the largest patch, we still have the alternative of running SM 6 I/O declarations through separate functions. They would be about the size of these patched spirv_compiler_emit_input() and spirv_compiler_emit_output() functions. The downside is it doesn't eliminate most private variables or fix the issue with register relative addressing. Those could still be done another way though. I would still keep the phase elimination.

  • Conor McCarthy added 3 commits

    added 3 commits

    • 2c7edcd6 - vkd3d-shader/spirv: Merge all shader IR fork and join phases into a single phase.
    • 2d8cc374 - vkd3d-shader/spirv: Remove the hull shader phase array.
    • 0157d461 - vkd3d-shader/spirv: Move the function declaration from...

    Compare with previous version

  • Conor McCarthy changed title from vkd3d-shader/spirv: Normalise inputs and outputs to the Shader Model 6 pattern. to vkd3d-shader/spirv: Merge all shader IR fork and join phases into a single phase.

    changed title from vkd3d-shader/spirv: Normalise inputs and outputs to the Shader Model 6 pattern. to vkd3d-shader/spirv: Merge all shader IR fork and join phases into a single phase.

  • Conor McCarthy changed the description

    changed the description

  • I feel like trying to accommodate the assembler (and disassembler) in vkd3d_shader_instruction is going to be a losing proposition at some point,

    I think so as well, yes. I also think we haven't quite reached that point yet though, and it seems helpful to not tie that particular change to shader model 6 support at this point.

    but in this case it at least doesn't seem right that it's in spirv.c; perhaps it could live in some "neutral" source file?

    Sure.

  • I feel like trying to accommodate the assembler (and disassembler) in vkd3d_shader_instruction is going to be a losing proposition at some point,

    I think so as well, yes. I also think we haven't quite reached that point yet though, and it seems helpful to not tie that particular change to shader model 6 support at this point.

    Eh, yes, agreed.

  • Conor McCarthy added 19 commits

    added 19 commits

    • 0157d461...240b2f96 - 16 commits from branch wine:master
    • 2be8a79f - vkd3d-shader/spirv: Merge all shader IR fork and join phases into a single phase.
    • 727bafee - vkd3d-shader/spirv: Remove the hull shader phase array.
    • f8ac000e - vkd3d-shader/spirv: Move the function declaration from...

    Compare with previous version

  • Conor McCarthy added 4 commits

    added 4 commits

    • 1f45a752 - vkd3d-shader/spirv: Merge all shader IR fork and join phases into a single phase.
    • 355fe75c - vkd3d-shader/spirv: Remove the hull shader phase array.
    • 3659de24 - vkd3d-shader/spirv: Move the function declaration from...
    • 1f0cc2db - vkd3d-shader: Introduce an internal sm6 signature structure.

    Compare with previous version

  • I added a normalise.c source file.

  • Conor McCarthy added 6 commits

    added 6 commits

    • cd36fe1e - vkd3d-shader/normalise: Merge all shader IR fork and join phases into a single phase.
    • bd431dc6 - vkd3d-shader/spirv: Remove the hull shader phase array.
    • a99b5502 - vkd3d-shader/spirv: Move the function declaration from...
    • a46b2430 - vkd3d-shader: Introduce an internal sm6 signature structure.
    • 7a3eb390 - vkd3d-shader/normalise: Normalise control point phase output registers to...
    • da41716a - vkd3d-shader/normalise: Insert hull shader control point input declarations if...

    Compare with previous version

  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit 1f45a752
94 static bool shader_register_clone_relative_addresses(struct vkd3d_shader_register *reg,
95 struct vkd3d_shader_instruction_array *instructions)
96 {
97 struct vkd3d_shader_src_param *src;
98 unsigned int i;
99
100 for (i = 0; i < ARRAY_SIZE(reg->idx) && reg->idx[i].offset != ~0u; ++i)
101 {
102 if (!reg->idx[i].rel_addr)
103 continue;
104
105 if (!(src = shader_src_param_allocator_get(&instructions->src_params, 1)))
106 return false;
107
108 memcpy(src, reg->idx[i].rel_addr, sizeof(*src));
109 reg->idx[i].rel_addr = src;
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit 1f45a752
  • 216 else
    217 normaliser->temp_is_instance_id |= 1ull << ins->dst->reg.idx[0].offset;
    218 *instruction_count = 0;
    219 return VKD3D_OK;
    220 }
    221
    222 if (normaliser->phase_body_idx == ~0u)
    223 normaliser->phase_body_idx = normaliser->instructions.count;
    224
    225 if (ins->handler_idx != VKD3DSIH_RET)
    226 return VKD3D_OK;
    227
    228 count = normaliser->instructions.count - normaliser->phase_body_idx;
    229
    230 if (!shader_instruction_array_reserve(&normaliser->instructions, count * (normaliser->instance_count - 1)))
    231 return VKD3D_ERROR_OUT_OF_MEMORY;
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading