vkd3d-shader/d3d-asm: Dump the input and output signatures.
At some point I would like to have an assembler for TPF, so that it's easier to write and modify tests. This is a first step in that direction, fixing some kind of format for serializing signatures in the comment at the beginning of the assembler code. I'm not decided yet on all details, so take this as an RFC for the moment.
Merge request reports
Activity
Could we perhaps do something like this?
.section ISGN dcl_param SV_Position, v0.xyz, float, POS dcl_param SV_IsFrontFace.x, v1.x, uint, FFACE dcl_param texcoord_2.xy, v2.xy dcl_param texcoord_3.xy, v3.xy, uint .section OSGN dcl_param SV_Target_0.xyzw, o0.xyzw, float, TARGET dcl_param SV_Target_1.xyzw, o1.xyzw, float, TARGET .section SHEX ...
I suppose the copy was done to avoid returning an oversized buffer, but I'd expect that the returned buffer won't be kept around for long, and the copy operation can be expensive. The caller can still decide to trim the buffer themselves, if so they wish.
Actually, a good chunk of the reason was simply avoiding the complication of transferring ownership of the underlying allocation from the string buffer to the vkd3d_shader_code structure. (I.e., vkd3d_shader_code_from_string_buffer(), essentially.)
Subject: [PATCH 2/3] vkd3d-shader/d3d-asm: Pass a string buffer to vkd3d_dxbc_binary_to_text().
Do we really need that? I guess this is mostly a consequence of the way vkd3d_dxbc_signature_to_text() works, but I'd prefer to keep that function internal to d3d_asm.c and call it from vkd3d_dxbc_binary_to_text().
While we're touching this, I'd suggest introducing a structure like this:
struct vsir_program { struct vkd3d_shader_desc shader_desc; struct vkd3d_shader_version shader_version; struct vkd3d_shader_instruction_array instructions; };
and possibly getting rid of struct vkd3d_shader_instruction_array in the long run:
struct vsir_program { struct vkd3d_shader_desc shader_desc; struct vkd3d_shader_version shader_version; struct vkd3d_shader_instruction *elements; size_t capacity; size_t count; struct vkd3d_shader_param_allocator src_params; struct vkd3d_shader_param_allocator dst_params; struct vkd3d_shader_immediate_constant_buffer **icbs; size_t icb_capacity; size_t icb_count; };
Could we perhaps do something like this?
.section ISGN dcl_param SV_Position, v0.xyz, float, POS dcl_param SV_IsFrontFace.x, v1.x, uint, FFACE dcl_param texcoord_2.xy, v2.xy dcl_param texcoord_3.xy, v3.xy, uint .section OSGN dcl_param SV_Target_0.xyzw, o0.xyzw, float, TARGET dcl_param SV_Target_1.xyzw, o1.xyzw, float, TARGET .section SHEX ...
My idea was to use comments (introduced by
//
) to stay compatible (and actually become more similar) to the native compiler. The format you suggest is technically incompatible in both directions. Is that intended?WRT the specific syntax, there is no slot for the register number. I guess that you take for implicit that the order is used, but AFAIU there are a few complications:
- a shader might use non contiguous registers: this can be worked around by adding a syntax to skip a register; it's not clear to me if large register numbers can be used for some definition of "large", but I'm not sure it's a good idea to be constrained by the syntax in this case;
- a register can be used more than twice, I think, with non-overlapping write masks;
- some parameters don't have a register number, because they use a specialized register type; I think that happens for depth and possibly a few others. In that case I think DXBC uses -1 internally.
I suppose the copy was done to avoid returning an oversized buffer, but I'd expect that the returned buffer won't be kept around for long, and the copy operation can be expensive. The caller can still decide to trim the buffer themselves, if so they wish.
Actually, a good chunk of the reason was simply avoiding the complication of transferring ownership of the underlying allocation from the string buffer to the vkd3d_shader_code structure. (I.e., vkd3d_shader_code_from_string_buffer(), essentially.)
It doesn't seem that complicated, though, does it? Helper
vkd3d_shader_code_from_string_buffer()
even abstracts it properly, but it seems to me we're willing to tolerate way worse encapsulation violations, aren't we?On the other hand, saving a buffer copy might be a better-than-negligible gain. I didn't measure specifically here, but
spirv.c
has a similar feature, and with callgrind I've seen a significant amount of time spent there (I have a few patches about that I intend to eventually submit).
My idea was to use comments (introduced by
//
) to stay compatible (and actually become more similar) to the native compiler. The format you suggest is technically incompatible in both directions. Is that intended?Yes. The assembler wouldn't have much to be compatible with, so I don't think we particularly care there, but the disassembler would probably need some kind of formatting flag or other compilation option to output in this format. Without that, I think we should just match the fxc output. I'd like to avoid assembling things from comments, at least by default.
WRT the specific syntax, there is no slot for the register number. I guess that you take for implicit that the order is used, but AFAIU there are a few complications:
"dcl_param SV_IsFrontFace.x, v1.x, uint, FFACE" would parse as
{ .semantic_name = "SV_IsFrontFace", .semantic_index = 0, .stream_index = 0, .sysval_semantic = VKD3D_SHADER_SV_IS_FRONT_FACE, .component_type = VKD3D_SHADER_COMPONENT_FLOAT, .register_index = 1, .mask = VKD3DSP_WRITEMASK_0, .used_mask = VKD3DSP_WRITEMASK_0, .min_precision = VKD3D_SHADER_MINIMUM_PRECISION_NONE, }
I.e., the register number would be taken from "v1". Am I missing something?
I.e., the register number would be taken from "v1". Am I missing something?
Ah, ok, I missed that. Still there are a few points I don't completely like:
- I don't like using
dcl_param
, which looks like the instructions that appear in the SHEX stream, while being a rather different beast; I find that confusing; rather, if you use the leading dot convention for meta instructions I'd use.param
; justparam
if you feel the leading dot doesn't apply there, but I'd avoid thedcl_
prefix. - I don't like assigning a write mask to a semantic name. Does this happen anywhere with the native compiler? It doesn't make a lot of sense to me. And anyway I guess the two masks are meant to be the write mask and the used mask, but I see what's the logic of assigning one to the semantic name and the other to the register name.
- WRT specifying registers, as I said sometimes other register names are used instead of
v
, for exampleoDepth
. In DXBC AFAICT this is marked by setting the register field to -1. Putting a register name there makes everything a little more complicated, because you have to know the mapping between the semantics and which register name they need; which is not terribly difficult, I think, but still is essentially for nothing (since a -1 is going to be there anyway) and the idea I have in mind is this is largely an internal feature, so it doesn't have to be terribly polished.
For all of these reasons I'd rather propose something as:
.param SV_IsFrontFace, x, 1, x, uint, FFACE
What do you think about that?
BTW, why doesn't
SV_Position
have any write mask? Are you assuming that it's the full one if it's not specified?- I don't like using
Ah, ok, I missed that. Still there are a few points I don't completely like:
- I don't like using
dcl_param
, which looks like the instructions that appear in the SHEX stream, while being a rather different beast; I find that confusing; rather, if you use the leading dot convention for meta instructions I'd use.param
; justparam
if you feel the leading dot doesn't apply there, but I'd avoid thedcl_
prefix.
Sure; either of those works for me.
- I don't like assigning a write mask to a semantic name. Does this happen anywhere with the native compiler? It doesn't make a lot of sense to me. And anyway I guess the two masks are meant to be the write mask and the used mask, but I see what's the logic of assigning one to the semantic name and the other to the register name.
The way I think of it, "SV_IsFrontFace.x" is the external (i.e., input layout) side, and "v1.x" is the internal (i.e., shader) side, and we're defining a mapping between them.
- WRT specifying registers, as I said sometimes other register names are used instead of
v
, for exampleoDepth
. In DXBC AFAICT this is marked by setting the register field to -1. Putting a register name there makes everything a little more complicated, because you have to know the mapping between the semantics and which register name they need; which is not terribly difficult, I think, but still is essentially for nothing (since a -1 is going to be there anyway) and the idea I have in mind is this is largely an internal feature, so it doesn't have to be terribly polished.
Well, it's going to be available through the public API, right?
For all of these reasons I'd rather propose something as:
.param SV_IsFrontFace, x, 1, x, uint, FFACE
What do you think about that?
Works for me. I think "SV_IsFrontFace.x" and "v1.x" would be more convenient when writing these, and arguably when looking at disassembled shaders in debug logs, but ultimately I'm not especially attached to a specific format.
BTW, why doesn't
SV_Position
have any write mask? Are you assuming that it's the full one if it's not specified?Right, much like how some of the other information is optional as well.
- I don't like using
The way I think of it, "SV_IsFrontFace.x" is the external (i.e., input layout) side, and "v1.x" is the internal (i.e., shader) side, and we're defining a mapping between them.
Yes, but which mask is the write mask and which one is the used mask? And why does it make sense to associate one to the external and one to the internal side? Having to choose I'd say that the write mask is the "external" one and the used mask is the "internal" one, but it's a weaker link than I'd put in a public interface.
Well, it's going to be available through the public API, right?
Yes, so it has to be properly specified; but for such an application I wouldn't be bothered if the specification is a bit rougher than usual (for example in requiring that a missing value is written as
-1
; or maybe something like_
if you prefer).Anyway, I can at least try to use the proper register name and see how complicated it becomes.
The way I think of it, "SV_IsFrontFace.x" is the external (i.e., input layout) side, and "v1.x" is the internal (i.e., shader) side, and we're defining a mapping between them.
Yes, but which mask is the write mask and which one is the used mask? And why does it make sense to associate one to the external and one to the internal side? Having to choose I'd say that the write mask is the "external" one and the used mask is the "internal" one, but it's a weaker link than I'd put in a public interface.
For example:
float4 main(in float3 i : INPUT) : OUTPUT { return float4(i.xy, 0.0, 1.0); }
which fxc compiles to:
// // Generated by Microsoft (R) HLSL Shader Compiler 10.0.10011.16384 // // // // Input signature: // // Name Index Mask Register SysValue Format Used // -------------------- ----- ------ -------- -------- ------- ------ // INPUT 0 xyz 0 NONE float xy // // // Output signature: // // Name Index Mask Register SysValue Format Used // -------------------- ----- ------ -------- -------- ------- ------ // OUTPUT 0 xyzw 0 NONE float xyzw // vs_5_0 dcl_globalFlags refactoringAllowed dcl_input v0.xy dcl_output o0.xyzw mov o0.xy, v0.xyxx mov o0.zw, l(0,0,0,1.000000) ret // Approximately 3 instruction slots used
The input signature would then look like this:
.section ISGN .param INPUT.xyz, v0.xy
I.e., "INPUT" matches what we would put in the input layout:
{ .SemanticName = "INPUT", .Format = DXGI_FORMAT_R32G32B32_FLOAT, [...] } D3D11_INPUT_ELEMENT_DESC;
while "v0.xy" simply matches "dcl_input_ps linear v0.xy". I.e., "Mask" is the external interface, while "Used" indicates what's actually used by the shader, largely analogous to the HLSL.
While we're touching this, I'd suggest introducing a structure like this:
struct vsir_program { struct vkd3d_shader_desc shader_desc; struct vkd3d_shader_version shader_version; struct vkd3d_shader_instruction_array instructions; };
Actually, I went ahead and wrote those patches; they should make it into an MR one of these days.
I suppose the copy was done to avoid returning an oversized buffer, but I'd expect that the returned buffer won't be kept around for long, and the copy operation can be expensive. The caller can still decide to trim the buffer themselves, if so they wish.
Actually, a good chunk of the reason was simply avoiding the complication of transferring ownership of the underlying allocation from the string buffer to the vkd3d_shader_code structure. (I.e., vkd3d_shader_code_from_string_buffer(), essentially.)
It doesn't seem that complicated, though, does it? Helper
vkd3d_shader_code_from_string_buffer()
even abstracts it properly, but it seems to me we're willing to tolerate way worse encapsulation violations, aren't we?On the other hand, saving a buffer copy might be a better-than-negligible gain. I didn't measure specifically here, but
spirv.c
has a similar feature, and with callgrind I've seen a significant amount of time spent there (I have a few patches about that I intend to eventually submit).Sure; the comment wasn't an objection.
added 6 commits
- 320977d3 - vkd3d-shader/d3d-asm: Do not make a copy of the buffer before returning it.
- a32d1a6d - vkd3d-shader/d3d-asm: Describe the ASM dialect with a bunch of flags instead of a plain enum.
- b5cff85e - vkd3d-shader/d3d-asm: Refactor dumping a write mask to a dedicated function.
- 5d689896 - vkd3d-shader/d3d-asm: Support emitting the shader signature.
- a864b944 - vkd3d-compiler: Add an option to emit the signature when disassembling.
- 4d4f0db7 - tests: Test emitting the signature.
Toggle commit listThe latest revision should integrate most of @hverbeet's suggestions, unless I missed something. There are still a few details to carve, so I'm leaving it as a draft. I'd value also @zfigura's feedback.
One specific point is the public API: should
VKD3D_SHADER_COMPILE_OPTION_EMIT_SIGNATURE
rather be a bitfield? Maybe allowing dumping the internal dialect too (documenting that the format is not considered stable)? Is there another option to use instead of introducing this new one?added 100 commits
-
4d4f0db7...14da4df9 - 94 commits from branch
wine:master
- e16fb15f - vkd3d-shader/d3d-asm: Do not make a copy of the buffer before returning it.
- 723dbc91 - vkd3d-shader/d3d-asm: Describe the ASM dialect with a bunch of flags instead of a plain enum.
- 0b751051 - vkd3d-shader/d3d-asm: Refactor dumping a write mask to a dedicated function.
- 2eaeb4a3 - vkd3d-shader/d3d-asm: Support emitting the shader signature.
- 70d19444 - vkd3d-compiler: Add an option to emit the signature when disassembling.
- 145ee0d9 - tests: Test emitting the signature.
Toggle commit list-
4d4f0db7...14da4df9 - 94 commits from branch
I think I like the syntax as it is, at least I agree with the changes that have been made, but:
-
Do we really want to invent a new set of names like that for sysval? Should we just clip VKD3D_SHADER_SV from the existing enum instead?
-
What's the plan for GS stream index and for minimum precision?
-
Any reason we're leaving out patch constant signatures from this series?
From patch 4/6:
- if (ret >= 0)
-
vkd3d_string_buffer_printf(buffer, ".section SHEX\n");
This feels weird, partly because it's in dump_signatures(), also because I don't see why it's SHEX and not SHDR.
-
Do we really want to invent a new set of names like that for sysval? Should we just clip VKD3D_SHADER_SV from the existing enum instead?
I copied them from native
/dumpbin
. We might still want to use our existing enum, sure; my personal opinion is that it's nice to have them a bit shorter.What's the plan for GS stream index and for minimum precision?
The general idea is that unspecified fields are treated as default; so we have to dump fields until all the rightmost ones are default. I was aware of minimum precision, but not of GS stream index. I'll try to add support for both.
Any reason we're leaving out patch constant signatures from this series?
No, it's just that I still have to add support for this too. My knowledge of shader signatures is still pretty young, so I'm learning while I'm doing.
From patch 4/6:
if (ret >= 0) vkd3d_string_buffer_printf(buffer, ".section SHEX\n");
This feels weird, partly because it's in dump_signatures(), also because I don't see why it's SHEX and not SHDR.
Ok, I can move this out. WRT to using fourccs, I don't have strong opinions, but I'd like to avoid overcomplicating the matter. So, for example, I would try to avoid two different tags depending on whether we're using minimum precision or not. My idea is that the directives I'm introducing specify the logical data; it's up to the assembler to decide which tags to use depending on the data. Using this point of view it might be preferable to rather use:
.section input .section output .section patch_constant .section code
However Henri explicitly suggested using fourccs, so he might have opinions here.
Edited by Giovanni MascellaniDo we really want to invent a new set of names like that for sysval? Should we just clip VKD3D_SHADER_SV from the existing enum instead?
I copied them from native
/dumpbin
. We might still want to use our existing enum, sure; my personal opinion is that it's nice to have them a bit shorter.Ah, I should have recognized that. If there's precedent I think that's fine, I just thought we were inventing names out of nowhere.
Any reason we're leaving out patch constant signatures from this series?
No, it's just that I still have to add support for this too. My knowledge of shader signatures is still pretty young, so I'm learning while I'm doing.
Fair enough.
FWIW, patch constant signatures are pretty simple: they're output for HS and input for DS. I think you can just copy any one of the tessellation shader pairs from d3d12.c for a simple example.
From patch 4/6:
if (ret >= 0) vkd3d_string_buffer_printf(buffer, ".section SHEX\n");
This feels weird, partly because it's in dump_signatures(), also because I don't see why it's SHEX and not SHDR.
Ok, I can move this out. WRT to using fourccs, I don't have strong opinions, but I'd like to avoid overcomplicating the matter. So, for example, I would try to avoid two different tags depending on whether we're using minimum precision or not. My idea is that the directives I'm introducing specify the logical data; it's up to the assembler to decide which tags to use depending on the data. Using this point of view it might be preferable to rather use:
.section input .section output .section patch_constant .section code
However Henri explicitly suggested using fourccs, so he might have opinions here.
Hrm, I see those were in the original suggestion.
What's weird about it is mostly that SHEX is the extended 5.0 version of SHDR, whereas ISGN and OSGN are the original versions. Granted, that is the default you get with 5.0, if you don't have minimum precision or GS stream index, but it feels like not the most obvious arbitrary choice.
mentioned in merge request !612
added 150 commits
-
145ee0d9...fef30dac - 144 commits from branch
wine:master
- 57747663 - vkd3d-shader/d3d-asm: Do not make a copy of the buffer before returning it.
- 4bded4e8 - vkd3d-shader/d3d-asm: Describe the ASM dialect with a bunch of flags instead of a plain enum.
- 0c1dc4f9 - vkd3d-shader/d3d-asm: Refactor dumping a write mask to a dedicated function.
- a7389513 - vkd3d-shader/d3d-asm: Support emitting the shader signature.
- ab69f7be - vkd3d-compiler: Add an option to emit the signature when disassembling.
- 2781548e - tests: Test emitting the signature.
Toggle commit list-
145ee0d9...fef30dac - 144 commits from branch
added 176 commits
-
b783a12b...500b61c3 - 170 commits from branch
wine:master
- 1d4cb338 - vkd3d-shader/d3d-asm: Do not make a copy of the buffer before returning it.
- 36187c97 - vkd3d-shader/d3d-asm: Describe the ASM dialect with a bunch of flags instead of a plain enum.
- afd83484 - vkd3d-shader/d3d-asm: Refactor dumping a write mask to a dedicated function.
- 6d5f9770 - vkd3d-shader/d3d-asm: Support emitting the shader signature.
- 2000a88c - vkd3d-compiler: Add an option to emit the signature when disassembling.
- 1e1a3905 - tests: Test emitting the signature.
Toggle commit list-
b783a12b...500b61c3 - 170 commits from branch
added 15 commits
-
8370c574...f866fb95 - 9 commits from branch
wine:master
- fa4a4958 - vkd3d-shader/d3d-asm: Do not make a copy of the buffer before returning it.
- 54f49b1f - vkd3d-shader/d3d-asm: Describe the ASM dialect with a bunch of flags instead of a plain enum.
- 1cdad240 - vkd3d-shader/d3d-asm: Refactor dumping a write mask to a dedicated function.
- c1fe8721 - vkd3d-shader/d3d-asm: Support emitting the shader signature.
- bc94c640 - vkd3d-compiler: Add an option to emit the signature when disassembling.
- 2ff8053d - tests: Test emitting the signature.
Toggle commit list-
8370c574...f866fb95 - 9 commits from branch
+static char *dump_write_mask(char buffer[5], uint32_t write_mask) +{ + unsigned int i = 0; + + if (write_mask & VKD3DSP_WRITEMASK_0) + buffer[i++] = 'x'; + if (write_mask & VKD3DSP_WRITEMASK_1) + buffer[i++] = 'y'; + if (write_mask & VKD3DSP_WRITEMASK_2) + buffer[i++] = 'z'; + if (write_mask & VKD3DSP_WRITEMASK_3) + buffer[i++] = 'w'; + + buffer[i++] = '\0'; + + return buffer; +}
Could we do something like this?:
static void shader_print_write_mask(struct vkd3d_d3d_asm_compiler *compiler, const char *prefix, uint32_t mask, const char *suffix) { unsigned int i = 0; char buffer[5]; if (mask & VKD3DSP_WRITEMASK_0) buffer[i++] = 'x'; if (mask & VKD3DSP_WRITEMASK_1) buffer[i++] = 'y'; if (mask & VKD3DSP_WRITEMASK_2) buffer[i++] = 'z'; if (mask & VKD3DSP_WRITEMASK_3) buffer[i++] = 'w'; buffer[i++] = '\0'; vkd3d_string_buffer_printf(&compiler->buffer, "%s.%s%s%s%s", prefix, compiler->colours.write_mask, buffer, compiler->colours.reset, suffix); }
In particular, I don't think this works:
+ if (element->register_index != -1) + { + vkd3d_string_buffer_printf(buffer, ".%s, v%d.%s", + dump_write_mask(tmp, element->mask), + element->register_index, dump_write_mask(tmp, element->used_mask)); + }
"tmp" would be overwritten before the vkd3d_string_buffer_printf() gets a chance to use it.
I think the shader_print_*() model would largely work well for outputting the actual signatures as well.
+ /** + * If \a value is nonzero, emit the input and output signature + * when disassembling a shader. + * + * This option is supported by vkd3d_shader_compile() when using + * D3D_BYTECODE, DXBC_TPF or DXBC_DXIL as source and D3D_ASM as + * target; it should not be enabled otherwise. + * + * \since 1.12 + */ + VKD3D_SHADER_COMPILE_OPTION_EMIT_SIGNATURE = 0x0000000b,
Do we need a new compilation option for that? I.e., could this be something like VKD3D_SHADER_COMPILE_OPTION_FORMATTING_IO_SIGNATURES instead?
+ if (flags & VSIR_ASM_FLAG_EMIT_SIGNATURE) + { + if ((result = dump_signatures(shader_desc, buffer)) < 0) + { + vkd3d_string_buffer_cleanup(buffer); + return result; + } + } + compiler.shader_version = *shader_version; shader_version = &compiler.shader_version; vkd3d_string_buffer_printf(buffer, "%s%s_%u_%u%s\n", compiler.colours.version,
I think it would make sense to keep the version as the first line of the output.
added 4 commits
Toggle commit listadded 4 commits
Toggle commit listShould we output input/output signatures for d3dbc shaders? Currently we do, and there's something to be said for that, but it does complicate things in a couple of ways:
- We wouldn't be able to assemble such signatures in any meaningful way for d3dbc targets. I.e., the assembler would largely just have to ignore them.
- The register names are slightly different. E.g. ps_2_0 has t# texture coordinate inputs and oC# colour outputs.
Yeah, right. Even for SM6 the current code is likely to produce bad results, because AFAIU many signature elements can be merged in a single one using
register_count
. I think the appropriate action right now is to emit the signature only for SM4-5, and leave the rest to the future if the need will arise. I will submit a MR for that.Mmh, I'm probably confused about SM6. At least for the moment these metadata are meant to represent whatever is stored in the DXBC container, so it shouldn't change anything with SM6. I'll just disable for SM1-3.
added 18 commits
-
3bb608ae...a0d52dc3 - 12 commits from branch
wine:master
- 94d64178 - vkd3d-shader/d3d-asm: Do not make a copy of the buffer before returning it.
- 9d993896 - vkd3d-shader/d3d-asm: Describe the ASM dialect with a bunch of flags instead of a plain enum.
- 01a687a7 - vkd3d-shader/d3d-asm: Refactor dumping a write mask to a dedicated function.
- 9aa86901 - vkd3d-shader/d3d-asm: Support emitting the shader signature.
- 9b0a9f63 - vkd3d-compiler: Add an option to emit the signature when disassembling.
- 6163d824 - tests: Test emitting the signature.
Toggle commit list-
3bb608ae...a0d52dc3 - 12 commits from branch