[PATCH v2 0/3] MR553: Draft: 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. -- v2: vkd3d-shader/d3d_asm: Dump the input and output signature in comments. vkd3d-shader/d3d-asm: Pass a string buffer to vkd3d_dxbc_binary_to_text(). https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> 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. --- libs/vkd3d-shader/d3d_asm.c | 14 +------------- libs/vkd3d-shader/vkd3d_shader_main.c | 10 ++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/libs/vkd3d-shader/d3d_asm.c b/libs/vkd3d-shader/d3d_asm.c index 0589bc421..02a8888e6 100644 --- a/libs/vkd3d-shader/d3d_asm.c +++ b/libs/vkd3d-shader/d3d_asm.c @@ -1964,7 +1964,6 @@ enum vkd3d_result vkd3d_dxbc_binary_to_text(const struct vkd3d_shader_instructio struct vkd3d_string_buffer *buffer; unsigned int indent, i, j; const char *indent_str; - void *code; static const struct vkd3d_d3d_asm_colours no_colours = { @@ -2063,18 +2062,7 @@ enum vkd3d_result vkd3d_dxbc_binary_to_text(const struct vkd3d_shader_instructio } } - if ((code = vkd3d_malloc(buffer->content_size))) - { - memcpy(code, buffer->buffer, buffer->content_size); - out->size = buffer->content_size; - out->code = code; - } - else - { - result = VKD3D_ERROR_OUT_OF_MEMORY; - } - - vkd3d_string_buffer_cleanup(buffer); + vkd3d_shader_code_from_string_buffer(out, buffer); return result; } diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index f12b11adb..6822f87d7 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -226,6 +226,16 @@ void vkd3d_string_buffer_release(struct vkd3d_string_buffer_cache *cache, struct cache->buffers[cache->count++] = buffer; } +void vkd3d_shader_code_from_string_buffer(struct vkd3d_shader_code *code, struct vkd3d_string_buffer *buffer) +{ + code->code = buffer->buffer; + code->size = buffer->content_size; + + buffer->buffer = NULL; + buffer->buffer_size = 0; + buffer->content_size = 0; +} + void vkd3d_shader_message_context_init(struct vkd3d_shader_message_context *context, enum vkd3d_shader_log_level log_level) { diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 199a47a76..5ac744c35 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1346,6 +1346,7 @@ void vkd3d_string_buffer_release(struct vkd3d_string_buffer_cache *list, struct vkd3d_string_buffer_trace_(buffer, __FUNCTION__) void vkd3d_string_buffer_trace_(const struct vkd3d_string_buffer *buffer, const char *function); int vkd3d_string_buffer_vprintf(struct vkd3d_string_buffer *buffer, const char *format, va_list args); +void vkd3d_shader_code_from_string_buffer(struct vkd3d_shader_code *code, struct vkd3d_string_buffer *buffer); struct vkd3d_bytecode_buffer { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/d3d_asm.c | 24 ++++++++++++------------ libs/vkd3d-shader/vkd3d_shader_main.c | 8 +++++++- libs/vkd3d-shader/vkd3d_shader_private.h | 2 +- 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/libs/vkd3d-shader/d3d_asm.c b/libs/vkd3d-shader/d3d_asm.c index 02a8888e6..96dcaa2eb 100644 --- a/libs/vkd3d-shader/d3d_asm.c +++ b/libs/vkd3d-shader/d3d_asm.c @@ -1953,7 +1953,7 @@ static void shader_dump_instruction(struct vkd3d_d3d_asm_compiler *compiler, enum vkd3d_result vkd3d_dxbc_binary_to_text(const struct vkd3d_shader_instruction_array *instructions, const struct vkd3d_shader_version *shader_version, const struct vkd3d_shader_compile_info *compile_info, - struct vkd3d_shader_code *out, enum vsir_asm_dialect dialect) + struct vkd3d_string_buffer *buffer, enum vsir_asm_dialect dialect) { enum vkd3d_shader_compile_option_formatting_flags formatting; struct vkd3d_d3d_asm_compiler compiler = @@ -1961,7 +1961,6 @@ enum vkd3d_result vkd3d_dxbc_binary_to_text(const struct vkd3d_shader_instructio .dialect = dialect, }; enum vkd3d_result result = VKD3D_OK; - struct vkd3d_string_buffer *buffer; unsigned int indent, i, j; const char *indent_str; @@ -2012,12 +2011,11 @@ enum vkd3d_result vkd3d_dxbc_binary_to_text(const struct vkd3d_shader_instructio else indent_str = ""; - buffer = &compiler.buffer; - vkd3d_string_buffer_init(buffer); + compiler.buffer = *buffer; compiler.shader_version = *shader_version; shader_version = &compiler.shader_version; - vkd3d_string_buffer_printf(buffer, "%s%s_%u_%u%s\n", compiler.colours.version, + vkd3d_string_buffer_printf(&compiler.buffer, "%s%s_%u_%u%s\n", compiler.colours.version, shader_get_type_prefix(shader_version->type), shader_version->major, shader_version->minor, compiler.colours.reset); @@ -2042,7 +2040,7 @@ enum vkd3d_result vkd3d_dxbc_binary_to_text(const struct vkd3d_shader_instructio for (j = 0; j < indent; ++j) { - vkd3d_string_buffer_printf(buffer, "%s", indent_str); + vkd3d_string_buffer_printf(&compiler.buffer, "%s", indent_str); } shader_dump_instruction(&compiler, ins); @@ -2062,7 +2060,7 @@ enum vkd3d_result vkd3d_dxbc_binary_to_text(const struct vkd3d_shader_instructio } } - vkd3d_shader_code_from_string_buffer(out, buffer); + *buffer = compiler.buffer; return result; } @@ -2071,13 +2069,15 @@ void vkd3d_shader_trace(const struct vkd3d_shader_instruction_array *instruction const struct vkd3d_shader_version *shader_version) { const char *p, *q, *end; - struct vkd3d_shader_code code; + struct vkd3d_string_buffer buffer; + + vkd3d_string_buffer_init(&buffer); - if (vkd3d_dxbc_binary_to_text(instructions, shader_version, NULL, &code, VSIR_ASM_VSIR) != VKD3D_OK) + if (vkd3d_dxbc_binary_to_text(instructions, shader_version, NULL, &buffer, VSIR_ASM_VSIR) != VKD3D_OK) return; - end = (const char *)code.code + code.size; - for (p = code.code; p < end; p = q) + end = (const char *)buffer.buffer + buffer.content_size; + for (p = buffer.buffer; p < end; p = q) { if (!(q = memchr(p, '\n', end - p))) q = end; @@ -2086,5 +2086,5 @@ void vkd3d_shader_trace(const struct vkd3d_shader_instruction_array *instruction TRACE(" %.*s", (int)(q - p), p); } - vkd3d_shader_free_shader_code(&code); + vkd3d_string_buffer_cleanup(&buffer); } diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 6822f87d7..48430b136 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -1566,8 +1566,14 @@ static int vkd3d_shader_parser_compile(struct vkd3d_shader_parser *parser, switch (compile_info->target_type) { case VKD3D_SHADER_TARGET_D3D_ASM: - ret = vkd3d_dxbc_binary_to_text(&parser->instructions, &parser->shader_version, compile_info, out, VSIR_ASM_D3D); + { + struct vkd3d_string_buffer buffer; + + vkd3d_string_buffer_init(&buffer); + ret = vkd3d_dxbc_binary_to_text(&parser->instructions, &parser->shader_version, compile_info, &buffer, VSIR_ASM_D3D); + vkd3d_shader_code_from_string_buffer(out, &buffer); break; + } case VKD3D_SHADER_TARGET_GLSL: if ((ret = scan_with_parser(&scan_info, message_context, &scan_descriptor_info, parser)) < 0) diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 5ac744c35..0a21976b9 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1332,7 +1332,7 @@ enum vsir_asm_dialect enum vkd3d_result vkd3d_dxbc_binary_to_text(const struct vkd3d_shader_instruction_array *instructions, const struct vkd3d_shader_version *shader_version, const struct vkd3d_shader_compile_info *compile_info, - struct vkd3d_shader_code *out, enum vsir_asm_dialect dialect); + struct vkd3d_string_buffer *buffer, enum vsir_asm_dialect dialect); void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer); struct vkd3d_string_buffer *vkd3d_string_buffer_get(struct vkd3d_string_buffer_cache *list); void vkd3d_string_buffer_init(struct vkd3d_string_buffer *buffer); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> The format does not correspond to native, but is meant to be easily parsed so that at some point we can introduce an assembler. --- libs/vkd3d-shader/d3d_asm.c | 120 ++++++++++++++++++++--- libs/vkd3d-shader/vkd3d_shader_main.c | 4 +- libs/vkd3d-shader/vkd3d_shader_private.h | 2 + tests/vkd3d_shader_api.c | 3 + 4 files changed, 116 insertions(+), 13 deletions(-) diff --git a/libs/vkd3d-shader/d3d_asm.c b/libs/vkd3d-shader/d3d_asm.c index 96dcaa2eb..69db78e11 100644 --- a/libs/vkd3d-shader/d3d_asm.c +++ b/libs/vkd3d-shader/d3d_asm.c @@ -1304,31 +1304,40 @@ static void shader_dump_reg_type(struct vkd3d_d3d_asm_compiler *compiler, shader_addline(buffer, ">"); } +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; +} + static void shader_dump_dst_param(struct vkd3d_d3d_asm_compiler *compiler, const struct vkd3d_shader_dst_param *param, bool is_declaration) { struct vkd3d_string_buffer *buffer = &compiler->buffer; uint32_t write_mask = param->write_mask; + char tmp[5]; shader_dump_register(compiler, ¶m->reg, is_declaration); if (write_mask && param->reg.dimension == VSIR_DIMENSION_VEC4) { - static const char write_mask_chars[] = "xyzw"; - if (param->reg.data_type == VKD3D_DATA_DOUBLE) write_mask = vsir_write_mask_32_from_64(write_mask); - shader_addline(buffer, ".%s", compiler->colours.write_mask); - if (write_mask & VKD3DSP_WRITEMASK_0) - shader_addline(buffer, "%c", write_mask_chars[0]); - if (write_mask & VKD3DSP_WRITEMASK_1) - shader_addline(buffer, "%c", write_mask_chars[1]); - if (write_mask & VKD3DSP_WRITEMASK_2) - shader_addline(buffer, "%c", write_mask_chars[2]); - if (write_mask & VKD3DSP_WRITEMASK_3) - shader_addline(buffer, "%c", write_mask_chars[3]); - shader_addline(buffer, "%s", compiler->colours.reset); + shader_addline(buffer, ".%s%s%s", compiler->colours.write_mask, + dump_write_mask(tmp, write_mask), compiler->colours.reset); } shader_print_precision(compiler, ¶m->reg); @@ -2088,3 +2097,90 @@ void vkd3d_shader_trace(const struct vkd3d_shader_instruction_array *instruction vkd3d_string_buffer_cleanup(&buffer); } + +static const char *get_sysval_semantic_name(enum vkd3d_shader_sysval_semantic semantic) +{ + switch (semantic) + { + case VKD3D_SHADER_SV_NONE: return "NONE"; + case VKD3D_SHADER_SV_POSITION: return "POS"; + case VKD3D_SHADER_SV_CLIP_DISTANCE: return "CLIPDST"; + case VKD3D_SHADER_SV_CULL_DISTANCE: return "CULLDST"; + case VKD3D_SHADER_SV_RENDER_TARGET_ARRAY_INDEX: return "RTINDEX"; + case VKD3D_SHADER_SV_VIEWPORT_ARRAY_INDEX: return "VPINDEX"; + case VKD3D_SHADER_SV_VERTEX_ID: return "VERTID"; + case VKD3D_SHADER_SV_PRIMITIVE_ID: return "PRIMID"; + case VKD3D_SHADER_SV_INSTANCE_ID: return "INSTID"; + case VKD3D_SHADER_SV_IS_FRONT_FACE: return "FFACE"; + case VKD3D_SHADER_SV_SAMPLE_INDEX: return "SAMPLE"; + case VKD3D_SHADER_SV_TESS_FACTOR_QUADEDGE: return "QUADEDGE"; + case VKD3D_SHADER_SV_TESS_FACTOR_QUADINT: return "QUADINT"; + case VKD3D_SHADER_SV_TESS_FACTOR_TRIEDGE: return "TRIEDGE"; + case VKD3D_SHADER_SV_TESS_FACTOR_TRIINT: return "TRIINT"; + case VKD3D_SHADER_SV_TESS_FACTOR_LINEDET: return "LINEDET"; + case VKD3D_SHADER_SV_TESS_FACTOR_LINEDEN: return "LINEDEN"; + case VKD3D_SHADER_SV_TARGET: return "TARGET"; + case VKD3D_SHADER_SV_DEPTH: return "DEPTH"; + case VKD3D_SHADER_SV_COVERAGE: return "COVERAGE"; + case VKD3D_SHADER_SV_DEPTH_GREATER_EQUAL: return "DEPTHGE"; + case VKD3D_SHADER_SV_DEPTH_LESS_EQUAL: return "DEPTHLE"; + case VKD3D_SHADER_SV_STENCIL_REF: return "STENCILREF"; + default: return "??"; + } +} + +static const char *get_component_type_name(enum vkd3d_shader_component_type type) +{ + switch (type) + { + case VKD3D_SHADER_COMPONENT_VOID: return "void"; + case VKD3D_SHADER_COMPONENT_UINT: return "uint"; + case VKD3D_SHADER_COMPONENT_INT: return "int"; + case VKD3D_SHADER_COMPONENT_FLOAT: return "float"; + case VKD3D_SHADER_COMPONENT_BOOL: return "bool"; + case VKD3D_SHADER_COMPONENT_DOUBLE: return "double"; + case VKD3D_SHADER_COMPONENT_UINT64: return "uint64"; + default: return "??"; + } +} + +static enum vkd3d_result dump_signature(const struct shader_signature *signature, + struct vkd3d_string_buffer *buffer, const char *prefix) +{ + unsigned int i; + char tmp[5]; + + for (i = 0; i < signature->element_count; ++i) + { + struct signature_element *element = &signature->elements[i]; + + vkd3d_string_buffer_printf(buffer, "// !%-6s %-30s %-2u %-4s %-2d %-10s %-6s %s\n", + prefix, element->semantic_name, element->semantic_index, dump_write_mask(tmp, element->mask), + element->register_index, get_sysval_semantic_name(element->sysval_semantic), + get_component_type_name(element->component_type), dump_write_mask(tmp, element->used_mask)); + } + + return VKD3D_OK; +} + +enum vkd3d_result vkd3d_dxbc_signature_to_text(const struct vkd3d_shader_parser *parser, + struct vkd3d_string_buffer *buffer) +{ + enum vkd3d_result ret = VKD3D_OK; + + if (parser->shader_desc.input_signature.element_count != 0) + { + vkd3d_string_buffer_printf(buffer, "// Input signature:\n"); + ret = dump_signature(&parser->shader_desc.input_signature, buffer, "input"); + vkd3d_string_buffer_printf(buffer, "//\n"); + } + + if (ret >= 0 && parser->shader_desc.output_signature.element_count != 0) + { + vkd3d_string_buffer_printf(buffer, "// Output signature:\n"); + ret = dump_signature(&parser->shader_desc.output_signature, buffer, "output"); + vkd3d_string_buffer_printf(buffer, "//\n"); + } + + return ret; +} diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 48430b136..651cef47e 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -1570,7 +1570,9 @@ static int vkd3d_shader_parser_compile(struct vkd3d_shader_parser *parser, struct vkd3d_string_buffer buffer; vkd3d_string_buffer_init(&buffer); - ret = vkd3d_dxbc_binary_to_text(&parser->instructions, &parser->shader_version, compile_info, &buffer, VSIR_ASM_D3D); + ret = vkd3d_dxbc_signature_to_text(parser, &buffer); + if (ret >= 0) + ret = vkd3d_dxbc_binary_to_text(&parser->instructions, &parser->shader_version, compile_info, &buffer, VSIR_ASM_D3D); vkd3d_shader_code_from_string_buffer(out, &buffer); break; } diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 0a21976b9..c1ec6fecd 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1333,6 +1333,8 @@ enum vsir_asm_dialect enum vkd3d_result vkd3d_dxbc_binary_to_text(const struct vkd3d_shader_instruction_array *instructions, const struct vkd3d_shader_version *shader_version, const struct vkd3d_shader_compile_info *compile_info, struct vkd3d_string_buffer *buffer, enum vsir_asm_dialect dialect); +enum vkd3d_result vkd3d_dxbc_signature_to_text(const struct vkd3d_shader_parser *parser, + struct vkd3d_string_buffer *buffer); void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer); struct vkd3d_string_buffer *vkd3d_string_buffer_get(struct vkd3d_string_buffer_cache *list); void vkd3d_string_buffer_init(struct vkd3d_string_buffer *buffer); diff --git a/tests/vkd3d_shader_api.c b/tests/vkd3d_shader_api.c index 018506a45..af499d4c0 100644 --- a/tests/vkd3d_shader_api.c +++ b/tests/vkd3d_shader_api.c @@ -263,6 +263,9 @@ static void test_d3dbc(void) }; static const char expected[] = "vs_1_0\n"; static const char expected_dcl_def[] = + "// Input signature:\n" + "// !input POSITION 0 xyzw 0 NONE float xyzw\n" + "//\n" "vs_1_1\n" "dcl_position0 v0\n" "def c0 = 1.00000000e+00, 0.00000000e+00, 0.00000000e+00, 1.00000000e+00\n"; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553
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: ```c 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: ```c 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; }; ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553#note_57442
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553#note_57450
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 ```c { .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? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553#note_57451
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`; just `param` if you feel the leading dot doesn't apply there, but I'd avoid the `dcl_` 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 example `oDepth`. 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? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553#note_57458
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`; just `param` if you feel the leading dot doesn't apply there, but I'd avoid the `dcl_` 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 example `oDepth`. 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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553#note_57465
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553#note_57469
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: ```hlsl 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: ```c { .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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553#note_57521
While we're touching this, I'd suggest introducing a structure like this:
```c 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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553#note_57743
``` 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). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553#note_57748
``` 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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/553#note_57843
participants (3)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)