vkd3d-shader/spirv: Merge all shader IR fork and join phases into a single phase.
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.
Merge request reports
Activity
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.
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.
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.
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.
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...
Toggle commit list-
0157d461...240b2f96 - 16 commits from branch
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.
Toggle commit listadded 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...
Toggle commit list- libs/vkd3d-shader/normalise.c 0 → 100644
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; - Comment on lines +108 to +109
changed this line in version 11 of the diff
- libs/vkd3d-shader/normalise.c 0 → 100644
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; changed this line in version 11 of the diff