There are two parts to this:
- First, a way to retrieve any signature from a DXBC shader. This is, I gather, generally useful for reflection, and it can be used as one source with which to implement d3d10 and d3d11 shader reflection APIs.
The rest of those APIs will need much more data to be exposed from d3d shaders, and while I was originally planning to expose that all in a single "vkd3d_shader_d3d_shader_info" structure, I think that signatures at least are a reasonable enough subset to have a dedicated structure. Moreover, I did not want to block sm1 support on too much API design.
- Second, signatures synthesized from sm1 byte code. This is conceptually a bit weird, because sm1 does not have signatures, but in terms of how these APIs are used by Wine (or other translation layers, as evidenced not least by the Vulkan test shader runner, which I have locally adapted for sm1 but not submitted yet) it fits rather nicely.
Because this is new API, it of course deserves plenty of discussion, especially the sm1 part. Several open questions which occurred to me while writing are:
1. Should we fix the mask (and used mask) for sm1 signatures as 0xf rather than 0? SPIR-V cares about this in order to declare I/O variables, which makes some amount of sense. In fact I have a patch in my local tree to change this, specifically for that purpose. However, we could also normalize it later.
2. If we do fix the mask as nonzero, should single-component semantics (fog, depth, etc...) be declared as 0x1 instead of 0xf?
3. Should BLENDINDICES get a UINT type? It's float in shader instructions (well, kind of, although in practice it's used as an array index of course), but the vertex attribute type is in practice "supposed" to be integer.
Part 1 of a series to implement sm1 -> spirv translation in vkd3d-shader, brought to you by late nights spent coding and rereading The Waste Land.
Ganga was sunken, and the limp leaves Waited for rain, while the black clouds Gathered far distant, over Himavant.
-- v3: vkd3d-shader: Synthesize signatures for d3dbc shaders. vkd3d-shader: Introduce an API to retrieve all signatures from DXBC shaders.
From: Zebediah Figura zfigura@codeweavers.com
It will be used from vkd3d_shader_scan() in following patches. --- libs/vkd3d-shader/vkd3d_shader_main.c | 74 +++++++++++++-------------- 1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 409d759c..e3c592f4 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -526,6 +526,43 @@ void vkd3d_shader_free_messages(char *messages) vkd3d_free(messages); }
+static bool vkd3d_shader_signature_from_shader_signature(struct vkd3d_shader_signature *signature, + const struct shader_signature *src) +{ + unsigned int i; + + signature->element_count = src->element_count; + if (!src->elements) + { + assert(!signature->element_count); + signature->elements = NULL; + return true; + } + + if (!(signature->elements = vkd3d_calloc(signature->element_count, sizeof(*signature->elements)))) + return false; + + for (i = 0; i < signature->element_count; ++i) + { + struct vkd3d_shader_signature_element *d = &signature->elements[i]; + struct signature_element *e = &src->elements[i]; + + d->semantic_name = e->semantic_name; + d->semantic_index = e->semantic_index; + d->stream_index = e->stream_index; + d->sysval_semantic = e->sysval_semantic; + d->component_type = e->component_type; + d->register_index = e->register_index; + if (e->register_count > 1) + FIXME("Arrayed elements are not supported yet.\n"); + d->mask = e->mask; + d->used_mask = e->used_mask; + d->min_precision = e->min_precision; + } + + return true; +} + struct vkd3d_shader_scan_context { struct vkd3d_shader_scan_descriptor_info *scan_descriptor_info; @@ -1401,43 +1438,6 @@ void vkd3d_shader_free_root_signature(struct vkd3d_shader_versioned_root_signatu desc->version = 0; }
-static bool vkd3d_shader_signature_from_shader_signature(struct vkd3d_shader_signature *signature, - const struct shader_signature *src) -{ - unsigned int i; - - signature->element_count = src->element_count; - if (!src->elements) - { - assert(!signature->element_count); - signature->elements = NULL; - return true; - } - - if (!(signature->elements = vkd3d_calloc(signature->element_count, sizeof(*signature->elements)))) - return false; - - for (i = 0; i < signature->element_count; ++i) - { - struct vkd3d_shader_signature_element *d = &signature->elements[i]; - struct signature_element *e = &src->elements[i]; - - d->semantic_name = e->semantic_name; - d->semantic_index = e->semantic_index; - d->stream_index = e->stream_index; - d->sysval_semantic = e->sysval_semantic; - d->component_type = e->component_type; - d->register_index = e->register_index; - if (e->register_count > 1) - FIXME("Arrayed elements are not supported yet.\n"); - d->mask = e->mask; - d->used_mask = e->used_mask; - d->min_precision = e->min_precision; - } - - return true; -} - void shader_signature_cleanup(struct shader_signature *signature) { vkd3d_free(signature->elements);
From: Zebediah Figura zfigura@codeweavers.com
--- include/vkd3d_shader.h | 79 ++++++++++- libs/vkd3d-shader/vkd3d_shader.map | 1 + libs/vkd3d-shader/vkd3d_shader_main.c | 42 ++++++ tests/vkd3d_shader_api.c | 187 ++++++++++++++++++++++++++ 4 files changed, 308 insertions(+), 1 deletion(-)
diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h index 859b8c79..6e8bed23 100644 --- a/include/vkd3d_shader.h +++ b/include/vkd3d_shader.h @@ -84,6 +84,11 @@ enum vkd3d_shader_structure_type * \since 1.3 */ VKD3D_SHADER_STRUCTURE_TYPE_DESCRIPTOR_OFFSET_INFO, + /** + * The structure is a vkd3d_shader_scan_signature_info structure. + * \since 1.x + */ + VKD3D_SHADER_STRUCTURE_TYPE_SCAN_SIGNATURE_INFO,
VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_STRUCTURE_TYPE), }; @@ -1550,6 +1555,54 @@ static inline uint32_t vkd3d_shader_create_swizzle(enum vkd3d_shader_swizzle_com | ((w & VKD3D_SHADER_SWIZZLE_MASK) << VKD3D_SHADER_SWIZZLE_SHIFT(3)); }
+/** + * A chained structure containing signatures scanned from a DXBC shader. + * + * All members (except for \ref type and \ref next) are output-only. + * + * This structure is passed to vkd3d_shader_scan() and extends + * vkd3d_shader_compile_info. + * + * Members of this structure are allocated by vkd3d-shader and should be freed + * with vkd3d_shader_free_scan_signature_info() when no longer needed. + * + * \since 1.x + */ +struct vkd3d_shader_scan_signature_info +{ + /** Must be set to VKD3D_SHADER_STRUCTURE_TYPE_SCAN_SIGNATURE_INFO. */ + enum vkd3d_shader_structure_type type; + /** Optional pointer to a structure containing further parameters. */ + const void *next; + + /** + * The parsed input signature embedded in a DXBC shader. + * The structure is zeroed for any other type of shader. + * + * The signature may contain pointers into the input shader, and should + * only be accessed while the input shader remains valid. + */ + struct vkd3d_shader_signature input; + + /** + * The parsed output signature embedded in a DXBC shader. + * The structure is zeroed for any other type of shader. + * + * The signature may contain pointers into the input shader, and should + * only be accessed while the input shader remains valid. + */ + struct vkd3d_shader_signature output; + + /** + * The parsed patch constant signature embedded in a DXBC hull shader. + * The structure is zeroed for any other type of shader. + * + * The signature may contain pointers into the input shader, and should + * only be accessed while the input shader remains valid. + */ + struct vkd3d_shader_signature patch_constant; +}; + #ifdef LIBVKD3D_SHADER_SOURCE # define VKD3D_SHADER_API VKD3D_EXPORT #else @@ -1624,6 +1677,7 @@ VKD3D_SHADER_API const enum vkd3d_shader_target_type *vkd3d_shader_get_supported * following chained structures: * - vkd3d_shader_interface_info * - vkd3d_shader_scan_descriptor_info + * - vkd3d_shader_scan_signature_info * - vkd3d_shader_spirv_domain_shader_target_info * - vkd3d_shader_spirv_target_info * - vkd3d_shader_transform_feedback_info @@ -1790,6 +1844,7 @@ VKD3D_SHADER_API int vkd3d_shader_convert_root_signature(struct vkd3d_shader_ver * \n * The DXBC_TPF scanner supports the following chained structures: * - vkd3d_shader_scan_descriptor_info + * - vkd3d_shader_scan_signature_info * \n * Although the \a compile_info parameter is read-only, chained structures * passed to this function need not be, and may serve as output parameters, @@ -1826,12 +1881,18 @@ VKD3D_SHADER_API void vkd3d_shader_free_scan_descriptor_info( struct vkd3d_shader_scan_descriptor_info *scan_descriptor_info);
/** - * Read the input signature of a compiled shader, returning a structural + * Read the input signature of a compiled DXBC shader, returning a structural * description which can be easily parsed by C code. * * This function parses a compiled shader. To parse a standalone root signature, * use vkd3d_shader_parse_root_signature(). * + * This function only parses DXBC shaders, and only retrieves the input + * signature. To retrieve signatures from other shader types, or other signature + * types, use vkd3d_shader_scan() and struct vkd3d_shader_scan_signature_info. + * This function returns the same input signature that is returned in + * struct vkd3d_shader_scan_signature_info. + * * \param dxbc Compiled byte code, in DXBC format. * * \param signature Output location in which the parsed root signature will be @@ -2021,6 +2082,19 @@ VKD3D_SHADER_API int vkd3d_shader_parse_dxbc(const struct vkd3d_shader_code *dxb VKD3D_SHADER_API int vkd3d_shader_serialize_dxbc(size_t section_count, const struct vkd3d_shader_dxbc_section_desc *sections, struct vkd3d_shader_code *dxbc, char **messages);
+/** + * Free members of struct vkd3d_shader_scan_signature_info allocated by + * vkd3d_shader_scan(). + * + * This function may free members of vkd3d_shader_scan_signature_info, but + * does not free the structure itself. + * + * \param info Scan information to free. + * + * \since 1.x + */ +VKD3D_SHADER_API void vkd3d_shader_free_scan_signature_info(struct vkd3d_shader_scan_signature_info *info); + #endif /* VKD3D_SHADER_NO_PROTOTYPES */
/** Type of vkd3d_shader_get_version(). */ @@ -2086,6 +2160,9 @@ typedef int (*PFN_vkd3d_shader_parse_dxbc)(const struct vkd3d_shader_code *dxbc, typedef int (*PFN_vkd3d_shader_serialize_dxbc)(size_t section_count, const struct vkd3d_shader_dxbc_section_desc *sections, struct vkd3d_shader_code *dxbc, char **messages);
+/** Type of vkd3d_shader_free_scan_signature_info(). \since 1.x */ +typedef void (*PFN_vkd3d_shader_free_scan_signature_info)(struct vkd3d_shader_scan_signature_info *info); + #ifdef __cplusplus } #endif /* __cplusplus */ diff --git a/libs/vkd3d-shader/vkd3d_shader.map b/libs/vkd3d-shader/vkd3d_shader.map index 6afc526a..8a596a80 100644 --- a/libs/vkd3d-shader/vkd3d_shader.map +++ b/libs/vkd3d-shader/vkd3d_shader.map @@ -8,6 +8,7 @@ global: vkd3d_shader_free_messages; vkd3d_shader_free_root_signature; vkd3d_shader_free_scan_descriptor_info; + vkd3d_shader_free_scan_signature_info; vkd3d_shader_free_shader_code; vkd3d_shader_free_shader_signature; vkd3d_shader_get_supported_source_types; diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index e3c592f4..a5ad986d 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -440,6 +440,18 @@ void vkd3d_shader_dump_shader(enum vkd3d_shader_source_type source_type, shader_get_source_type_suffix(source_type), shader->code, shader->size); }
+static void init_scan_signature_info(const struct vkd3d_shader_compile_info *info) +{ + struct vkd3d_shader_scan_signature_info *signature_info; + + if ((signature_info = vkd3d_find_struct(info->next, SCAN_SIGNATURE_INFO))) + { + memset(&signature_info->input, 0, sizeof(signature_info->input)); + memset(&signature_info->output, 0, sizeof(signature_info->output)); + memset(&signature_info->patch_constant, 0, sizeof(signature_info->patch_constant)); + } +} + bool vkd3d_shader_parser_init(struct vkd3d_shader_parser *parser, struct vkd3d_shader_message_context *message_context, const char *source_name, const struct vkd3d_shader_version *version, const struct vkd3d_shader_parser_ops *ops, @@ -1107,6 +1119,7 @@ static int scan_with_parser(const struct vkd3d_shader_compile_info *compile_info struct vkd3d_shader_message_context *message_context, struct vkd3d_shader_parser *parser) { struct vkd3d_shader_scan_descriptor_info *scan_descriptor_info; + struct vkd3d_shader_scan_signature_info *signature_info; struct vkd3d_shader_instruction *instruction; struct vkd3d_shader_scan_context context; int ret = VKD3D_OK; @@ -1117,6 +1130,7 @@ static int scan_with_parser(const struct vkd3d_shader_compile_info *compile_info scan_descriptor_info->descriptors = NULL; scan_descriptor_info->descriptor_count = 0; } + signature_info = vkd3d_find_struct(compile_info->next, SCAN_SIGNATURE_INFO);
vkd3d_shader_scan_context_init(&context, compile_info, scan_descriptor_info, message_context);
@@ -1136,6 +1150,21 @@ static int scan_with_parser(const struct vkd3d_shader_compile_info *compile_info } }
+ if (!ret && signature_info) + { + if (!vkd3d_shader_signature_from_shader_signature(&signature_info->input, &parser->shader_desc.input_signature) + || !vkd3d_shader_signature_from_shader_signature(&signature_info->output, + &parser->shader_desc.output_signature) + || !vkd3d_shader_signature_from_shader_signature(&signature_info->patch_constant, + &parser->shader_desc.patch_constant_signature)) + { + vkd3d_shader_free_scan_signature_info(signature_info); + if (scan_descriptor_info) + vkd3d_shader_free_scan_descriptor_info(scan_descriptor_info); + ret = VKD3D_ERROR_OUT_OF_MEMORY; + } + } + vkd3d_shader_scan_context_cleanup(&context); return ret; } @@ -1189,6 +1218,8 @@ int vkd3d_shader_scan(const struct vkd3d_shader_compile_info *compile_info, char if ((ret = vkd3d_shader_validate_compile_info(compile_info, false)) < 0) return ret;
+ init_scan_signature_info(compile_info); + vkd3d_shader_message_context_init(&message_context, compile_info->log_level);
switch (compile_info->source_type) @@ -1342,6 +1373,8 @@ int vkd3d_shader_compile(const struct vkd3d_shader_compile_info *compile_info, if ((ret = vkd3d_shader_validate_compile_info(compile_info, true)) < 0) return ret;
+ init_scan_signature_info(compile_info); + vkd3d_shader_message_context_init(&message_context, compile_info->log_level);
switch (compile_info->source_type) @@ -1376,6 +1409,15 @@ void vkd3d_shader_free_scan_descriptor_info(struct vkd3d_shader_scan_descriptor_ vkd3d_free(scan_descriptor_info->descriptors); }
+void vkd3d_shader_free_scan_signature_info(struct vkd3d_shader_scan_signature_info *info) +{ + TRACE("info %p.\n", info); + + vkd3d_shader_free_shader_signature(&info->input); + vkd3d_shader_free_shader_signature(&info->output); + vkd3d_shader_free_shader_signature(&info->patch_constant); +} + void vkd3d_shader_free_shader_code(struct vkd3d_shader_code *shader_code) { TRACE("shader_code %p.\n", shader_code); diff --git a/tests/vkd3d_shader_api.c b/tests/vkd3d_shader_api.c index 54e5d5ba..318e5068 100644 --- a/tests/vkd3d_shader_api.c +++ b/tests/vkd3d_shader_api.c @@ -401,6 +401,192 @@ static void test_dxbc(void) vkd3d_shader_free_shader_code(&dxbc); }
+static void check_signature_element(const struct vkd3d_shader_signature_element *element, + const struct vkd3d_shader_signature_element *expect) +{ + ok(!strcmp(element->semantic_name, expect->semantic_name), "Got semantic name %s.\n", element->semantic_name); + ok(element->semantic_index == expect->semantic_index, "Got semantic index %u.\n", element->semantic_index); + ok(element->stream_index == expect->stream_index, "Got stream index %u.\n", element->stream_index); + ok(element->sysval_semantic == expect->sysval_semantic, "Got sysval semantic %#x.\n", element->sysval_semantic); + ok(element->component_type == expect->component_type, "Got component type %#x.\n", element->component_type); + ok(element->register_index == expect->register_index, "Got register index %u.\n", element->register_index); + ok(element->mask == expect->mask, "Got mask %#x.\n", element->mask); + todo_if (expect->used_mask != expect->mask) + ok(element->used_mask == expect->used_mask, "Got used mask %#x.\n", element->used_mask); + ok(element->min_precision == expect->min_precision, "Got minimum precision %#x.\n", element->min_precision); +} + +static void test_scan_signatures(void) +{ + struct vkd3d_shader_scan_signature_info signature_info = {.type = VKD3D_SHADER_STRUCTURE_TYPE_SCAN_SIGNATURE_INFO}; + struct vkd3d_shader_hlsl_source_info hlsl_info = {.type = VKD3D_SHADER_STRUCTURE_TYPE_HLSL_SOURCE_INFO}; + struct vkd3d_shader_compile_info compile_info = {.type = VKD3D_SHADER_STRUCTURE_TYPE_COMPILE_INFO}; + struct vkd3d_shader_code dxbc; + size_t i, j; + int rc; + + static const char vs1_source[] = + "void main(\n" + " in float4 a : apple,\n" + " out float4 b : banana2,\n" + " inout float4 c : color,\n" + " inout float4 d : depth,\n" + " inout float4 e : sv_position,\n" + " in uint3 f : fruit,\n" + " inout bool2 g : grape,\n" + " in int h : honeydew,\n" + " in uint i : sv_vertexid)\n" + "{\n" + " b.yw = a.xz;\n" + "}"; + + static const struct vkd3d_shader_signature_element vs1_inputs[] = + { + {"apple", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0x5}, + {"color", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 1, 0xf, 0xf}, + {"depth", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0xf, 0xf}, + {"sv_position", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 3, 0xf, 0xf}, + {"fruit", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_UINT, 4, 0x7}, + {"grape", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_UINT, 5, 0x3, 0x3}, + {"honeydew", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_INT, 6, 0x1}, + {"sv_vertexid", 0, 0, VKD3D_SHADER_SV_VERTEX_ID, VKD3D_SHADER_COMPONENT_UINT, 7, 0x1}, + }; + + static const struct vkd3d_shader_signature_element vs1_outputs[] = + { + {"banana", 2, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xa}, + {"color", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 1, 0xf, 0xf}, + {"depth", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0xf, 0xf}, + {"sv_position", 0, 0, VKD3D_SHADER_SV_POSITION, VKD3D_SHADER_COMPONENT_FLOAT, 3, 0xf, 0xf}, + {"grape", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_UINT, 4, 0x3, 0x3}, + }; + + static const char vs2_source[] = + "void main(inout float4 pos : position)\n" + "{\n" + "}"; + + static const struct vkd3d_shader_signature_element vs2_inputs[] = + { + {"position", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + }; + + static const struct vkd3d_shader_signature_element vs2_outputs[] = + { + {"position", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + }; + + static const char ps1_source[] = + "void main(\n" + " in float2 a : apple,\n" + " out float4 b : sv_target2,\n" + " out float c : sv_depth,\n" + " in float4 d : position,\n" + " in float4 e : sv_position)\n" + "{\n" + " b = d;\n" + " c = 0;\n" + "}"; + + static const struct vkd3d_shader_signature_element ps1_inputs[] = + { + {"apple", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0x3}, + {"position", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 1, 0xf, 0xf}, + {"sv_position", 0, 0, VKD3D_SHADER_SV_POSITION, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0xf}, + }; + + static const struct vkd3d_shader_signature_element ps1_outputs[] = + { + {"sv_target", 2, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0xf, 0xf}, + {"sv_depth", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, ~0u, 0x1, 0x1}, + }; + + static const char cs1_source[] = + "[numthreads(1, 1, 1)]\n" + "void main(\n" + " in uint a : sv_dispatchthreadid,\n" + " in uint b : sv_groupid,\n" + " in uint c : sv_groupthreadid)\n" + "{\n" + "}"; + + static const struct + { + const char *source; + const char *profile; + const struct vkd3d_shader_signature_element *inputs; + size_t input_count; + const struct vkd3d_shader_signature_element *outputs; + size_t output_count; + const struct vkd3d_shader_signature_element *patch_constants; + size_t patch_constant_count; + } + tests[] = + { + {vs1_source, "vs_4_0", vs1_inputs, ARRAY_SIZE(vs1_inputs), vs1_outputs, ARRAY_SIZE(vs1_outputs)}, + {vs2_source, "vs_4_0", vs2_inputs, ARRAY_SIZE(vs2_inputs), vs2_outputs, ARRAY_SIZE(vs2_outputs)}, + {ps1_source, "ps_4_0", ps1_inputs, ARRAY_SIZE(ps1_inputs), ps1_outputs, ARRAY_SIZE(ps1_outputs)}, + {cs1_source, "cs_5_0", NULL, 0, NULL, 0}, + }; + + for (i = 0; i < ARRAY_SIZE(tests); ++i) + { + vkd3d_test_push_context("test %u", i); + + compile_info.source.code = tests[i].source; + compile_info.source.size = strlen(tests[i].source); + compile_info.source_type = VKD3D_SHADER_SOURCE_HLSL; + compile_info.target_type = VKD3D_SHADER_TARGET_DXBC_TPF; + compile_info.log_level = VKD3D_SHADER_LOG_INFO; + + compile_info.next = &hlsl_info; + hlsl_info.profile = tests[i].profile; + + rc = vkd3d_shader_compile(&compile_info, &dxbc, NULL); + ok(rc == VKD3D_OK, "Got unexpected error code %d.\n", rc); + + compile_info.source_type = VKD3D_SHADER_SOURCE_DXBC_TPF; + compile_info.source = dxbc; + + compile_info.next = &signature_info; + + rc = vkd3d_shader_scan(&compile_info, NULL); + ok(rc == VKD3D_OK, "Got unexpected error code %d.\n", rc); + + ok(signature_info.input.element_count == tests[i].input_count, + "Got input count %u.\n", signature_info.input.element_count); + for (j = 0; j < signature_info.input.element_count; ++j) + { + vkd3d_test_push_context("input %u", j); + check_signature_element(&signature_info.input.elements[j], &tests[i].inputs[j]); + vkd3d_test_pop_context(); + } + + ok(signature_info.output.element_count == tests[i].output_count, + "Got output count %u.\n", signature_info.output.element_count); + for (j = 0; j < signature_info.output.element_count; ++j) + { + vkd3d_test_push_context("output %u", j); + check_signature_element(&signature_info.output.elements[j], &tests[i].outputs[j]); + vkd3d_test_pop_context(); + } + + ok(signature_info.patch_constant.element_count == tests[i].patch_constant_count, + "Got patch constant count %u.\n", signature_info.patch_constant.element_count); + for (j = 0; j < signature_info.patch_constant.element_count; ++j) + { + vkd3d_test_push_context("patch constant %u", j); + check_signature_element(&signature_info.patch_constant.elements[j], &tests[i].patch_constants[j]); + vkd3d_test_pop_context(); + } + + vkd3d_shader_free_scan_signature_info(&signature_info); + vkd3d_shader_free_shader_code(&dxbc); + + vkd3d_test_pop_context(); + } +} + START_TEST(vkd3d_shader_api) { setlocale(LC_ALL, ""); @@ -410,4 +596,5 @@ START_TEST(vkd3d_shader_api) run_test(test_version); run_test(test_d3dbc); run_test(test_dxbc); + run_test(test_scan_signatures); }
From: Zebediah Figura zfigura@codeweavers.com
--- include/vkd3d_shader.h | 45 +++- libs/vkd3d-shader/d3dbc.c | 258 +++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 3 + tests/vkd3d_shader_api.c | 154 +++++++++++++- 4 files changed, 453 insertions(+), 7 deletions(-)
diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h index 6e8bed23..7540b334 100644 --- a/include/vkd3d_shader.h +++ b/include/vkd3d_shader.h @@ -1556,7 +1556,8 @@ static inline uint32_t vkd3d_shader_create_swizzle(enum vkd3d_shader_swizzle_com }
/** - * A chained structure containing signatures scanned from a DXBC shader. + * A chained structure containing signatures scanned from a DXBC shader, or + * synthesized from legacy Direct3D bytecode instructions. * * All members (except for \ref type and \ref next) are output-only. * @@ -1566,6 +1567,48 @@ static inline uint32_t vkd3d_shader_create_swizzle(enum vkd3d_shader_swizzle_com * Members of this structure are allocated by vkd3d-shader and should be freed * with vkd3d_shader_free_scan_signature_info() when no longer needed. * + * Signature elements are synthesized from legacy Direct3D bytecode as follows: + * - The \ref vkd3d_shader_signature_element.semantic_name field is set to an + * uppercase string corresponding to the HLSL name for the usage, e.g. + * "POSITION", "BLENDWEIGHT", "COLOR", "PSIZE", etc. + * - The \ref vkd3d_shader_signature_element.semantic_index field is set to the + * usage index. + * - The \ref vkd3d_shader_signature_element.stream_index is always 0. + * - The \ref vkd3d_shader_signature_element.sysval_semantic field is set as + * follows: + * - For position outputs from vertex shaders, it is set to + * VKD3D_SHADER_SV_POSITION. + * - For shader model 3 pixel position inputs (i.e. VPOS, but not POSITION), + * it is set to VKD3D_SHADER_SV_POSITION. + * - For shader model 3 face inputs (i.e. VFACE), it is set to + * VKD3D_SHADER_SV_IS_FRONT_FACE. + * - For all other elements, it is set to VKD3D_SHADER_SV_NONE. + * - The \ref vkd3d_shader_signature_element.component_type field is always set + * to VKD3D_SHADER_COMPONENT_FLOAT. + * - The \ref vkd3d_shader_signature_element.register_index field is set to the + * bytecode register index. + * Note that for shader model 1 and 2 shaders (excepting vertex shader + * inputs), the register index of colour and texture coordinate registers will + * be equal to the usage index, and hence may not be unique. + * - The \ref vkd3d_shader_signature_element.mask field is set to the mask given + * in the DCL instruction, if one is present. If there is no DCL instruction + * for this semantic, it is set to the same value as the \ref used_mask field. + * For the scalar registers vFace, oDepth, oFog, and oPts, it is always 1. + * - The \ref vkd3d_shader_signature_element.used_mask field is set to the + * combined mask used by shader instructions (other than DCL). For scalar + * registers, it is always 1. + * - The \ref vkd3d_shader_signature_element.min_precision field is always + * VKD3D_SHADER_MINIMUM_PRECISION_NONE. + * + * Signature elements are synthesized for any input or output register declared + * or used in a legacy Direct3D bytecode shader, including the following: + * - Shader model 1 and 2 colour and texture coordinate registers. + * - The shader model 1 pixel shader output register. + * - Shader model 1 and 2 vertex shader output registers (position, fog, and + * point size). + * - Shader model 3 pixel shader system value input registers (pixel position + * and face). + * * \since 1.x */ struct vkd3d_shader_scan_signature_info diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 823df6f6..3fb4d06f 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -214,6 +214,8 @@ struct vkd3d_shader_sm1_parser bool abort;
struct vkd3d_shader_parser p; + + size_t input_signature_capacity, output_signature_capacity; };
/* This table is not order or position dependent. */ @@ -488,6 +490,245 @@ static void shader_sm1_parse_dst_param(uint32_t param, const struct vkd3d_shader dst->shift = (param & VKD3D_SM1_DSTSHIFT_MASK) >> VKD3D_SM1_DSTSHIFT_SHIFT; }
+static struct signature_element *find_signature_element(const struct shader_signature *signature, + const char *semantic_name, unsigned int semantic_index) +{ + struct signature_element *e = signature->elements; + unsigned int i; + + for (i = 0; i < signature->element_count; ++i) + { + if (!ascii_strcasecmp(e[i].semantic_name, semantic_name) + && e[i].semantic_index == semantic_index) + return &e[i]; + } + + return NULL; +} + +static struct signature_element *find_signature_element_by_register_index( + const struct shader_signature *signature, unsigned int register_index) +{ + struct signature_element *e = signature->elements; + unsigned int i; + + for (i = 0; i < signature->element_count; ++i) + { + if (e[i].register_index == register_index) + return &e[i]; + } + + return NULL; +} + +static bool add_signature_element(struct vkd3d_shader_sm1_parser *sm1, bool output, + const char *name, unsigned int index, enum vkd3d_shader_sysval_semantic sysval, + unsigned int register_index, bool is_dcl, unsigned int mask) +{ + struct shader_signature *signature; + struct signature_element *element; + + if (output) + signature = &sm1->p.shader_desc.output_signature; + else + signature = &sm1->p.shader_desc.input_signature; + + if ((element = find_signature_element(signature, name, index))) + { + element->mask |= mask; + if (!is_dcl) + element->used_mask |= mask; + return true; + } + + if (!vkd3d_array_reserve((void **)&signature->elements, &signature->elements_capacity, + signature->element_count + 1, sizeof(*signature->elements))) + return false; + element = &signature->elements[signature->element_count++]; + + element->semantic_name = name; + element->semantic_index = index; + element->stream_index = 0; + element->sysval_semantic = sysval; + element->component_type = VKD3D_SHADER_COMPONENT_FLOAT; + element->register_index = register_index; + element->register_count = 1; + element->mask = mask; + element->used_mask = is_dcl ? 0 : mask; + element->min_precision = VKD3D_SHADER_MINIMUM_PRECISION_NONE; + + return true; +} + +static void add_signature_mask(struct vkd3d_shader_sm1_parser *sm1, bool output, + unsigned int register_index, unsigned int mask) +{ + struct shader_signature *signature; + struct signature_element *element; + + if (output) + signature = &sm1->p.shader_desc.output_signature; + else + signature = &sm1->p.shader_desc.input_signature; + + if (!(element = find_signature_element_by_register_index(signature, register_index))) + { + vkd3d_shader_parser_error(&sm1->p, VKD3D_SHADER_ERROR_D3DBC_UNDECLARED_SEMANTIC, + "%s register %u was used without being declared.", output ? "Output" : "Input", register_index); + return; + } + + element->used_mask |= mask; +} + +static bool add_signature_element_from_register(struct vkd3d_shader_sm1_parser *sm1, + const struct vkd3d_shader_register *reg, bool is_dcl, unsigned int mask) +{ + unsigned int register_index = reg->idx[0].offset; + + switch (reg->type) + { + case VKD3DSPR_TEMP: + if (sm1->p.shader_version.type == VKD3D_SHADER_TYPE_PIXEL + && sm1->p.shader_version.major == 1 && !register_index) + return add_signature_element(sm1, true, "COLOR", 0, VKD3D_SHADER_SV_NONE, 0, is_dcl, mask); + return true; + + case VKD3DSPR_INPUT: + /* For vertex shaders or sm3 pixel shaders, we should have already + * had a DCL instruction. Otherwise, this is a colour input. */ + if (sm1->p.shader_version.type == VKD3D_SHADER_TYPE_VERTEX || sm1->p.shader_version.major == 3) + { + add_signature_mask(sm1, false, register_index, mask); + return true; + } + return add_signature_element(sm1, false, "COLOR", register_index, + VKD3D_SHADER_SV_NONE, register_index, is_dcl, mask); + + case VKD3DSPR_TEXTURE: + /* For vertex shaders, this is ADDR. */ + if (sm1->p.shader_version.type == VKD3D_SHADER_TYPE_VERTEX) + return true; + return add_signature_element(sm1, false, "TEXCOORD", register_index, + VKD3D_SHADER_SV_NONE, register_index, is_dcl, mask); + + case VKD3DSPR_OUTPUT: + if (sm1->p.shader_version.type == VKD3D_SHADER_TYPE_VERTEX) + { + /* For sm < 2 vertex shaders, this is TEXCRDOUT. + * + * For sm3 vertex shaders, this is OUTPUT, but we already + * should have had a DCL instruction. */ + if (sm1->p.shader_version.major == 3) + { + add_signature_mask(sm1, true, register_index, mask); + return true; + } + return add_signature_element(sm1, true, "TEXCOORD", register_index, + VKD3D_SHADER_SV_NONE, register_index, is_dcl, mask); + } + /* fall through */ + + case VKD3DSPR_ATTROUT: + case VKD3DSPR_COLOROUT: + return add_signature_element(sm1, true, "COLOR", register_index, + VKD3D_SHADER_SV_NONE, register_index, is_dcl, mask); + + case VKD3DSPR_DEPTHOUT: + return add_signature_element(sm1, true, "DEPTH", 0, + VKD3D_SHADER_SV_NONE, register_index, is_dcl, 0x1); + + case VKD3DSPR_RASTOUT: + switch (register_index) + { + case 0: + return add_signature_element(sm1, true, "POSITION", 0, + VKD3D_SHADER_SV_POSITION, register_index, is_dcl, mask); + + case 1: + return add_signature_element(sm1, true, "FOG", 0, + VKD3D_SHADER_SV_NONE, register_index, is_dcl, 0x1); + + case 2: + return add_signature_element(sm1, true, "PSIZE", 0, + VKD3D_SHADER_SV_NONE, register_index, is_dcl, 0x1); + + default: + vkd3d_shader_parser_error(&sm1->p, VKD3D_SHADER_ERROR_D3DBC_INVALID_REGISTER_INDEX, + "Invalid rasterizer output index %u.", register_index); + return true; + } + + case VKD3DSPR_MISCTYPE: + switch (register_index) + { + case 0: + return add_signature_element(sm1, false, "VPOS", 0, + VKD3D_SHADER_SV_POSITION, register_index, is_dcl, mask); + + case 1: + return add_signature_element(sm1, false, "VFACE", 0, + VKD3D_SHADER_SV_IS_FRONT_FACE, register_index, is_dcl, 0x1); + + default: + vkd3d_shader_parser_error(&sm1->p, VKD3D_SHADER_ERROR_D3DBC_INVALID_REGISTER_INDEX, + "Invalid miscellaneous fragment input index %u.", register_index); + return true; + } + + default: + return true; + } +} + +static bool add_signature_element_from_semantic(struct vkd3d_shader_sm1_parser *sm1, + const struct vkd3d_shader_semantic *semantic) +{ + const struct vkd3d_shader_register *reg = &semantic->resource.reg.reg; + enum vkd3d_shader_sysval_semantic sysval = VKD3D_SHADER_SV_NONE; + unsigned int mask = semantic->resource.reg.write_mask; + bool output; + + static const char sm1_semantic_names[][13] = + { + [VKD3D_DECL_USAGE_POSITION ] = "POSITION", + [VKD3D_DECL_USAGE_BLEND_WEIGHT ] = "BLENDWEIGHT", + [VKD3D_DECL_USAGE_BLEND_INDICES] = "BLENDINDICES", + [VKD3D_DECL_USAGE_NORMAL ] = "NORMAL", + [VKD3D_DECL_USAGE_PSIZE ] = "PSIZE", + [VKD3D_DECL_USAGE_TEXCOORD ] = "TEXCOORD", + [VKD3D_DECL_USAGE_TANGENT ] = "TANGENT", + [VKD3D_DECL_USAGE_BINORMAL ] = "BINORMAL", + [VKD3D_DECL_USAGE_TESS_FACTOR ] = "TESSFACTOR", + [VKD3D_DECL_USAGE_POSITIONT ] = "POSITIONT", + [VKD3D_DECL_USAGE_COLOR ] = "COLOR", + [VKD3D_DECL_USAGE_FOG ] = "FOG", + [VKD3D_DECL_USAGE_DEPTH ] = "DEPTH", + [VKD3D_DECL_USAGE_SAMPLE ] = "SAMPLE", + }; + + if (reg->type == VKD3DSPR_OUTPUT) + output = true; + else if (reg->type == VKD3DSPR_INPUT || reg->type == VKD3DSPR_TEXTURE) + output = false; + else /* vpos and vface don't have a semantic. */ + return add_signature_element_from_register(sm1, reg, true, mask); + + /* sm2 pixel shaders use DCL but don't provide a semantic. */ + if (sm1->p.shader_version.type == VKD3D_SHADER_TYPE_PIXEL && sm1->p.shader_version.major == 2) + return add_signature_element_from_register(sm1, reg, true, mask); + + /* With the exception of vertex POSITION output, none of these are system + * values. Pixel POSITION input is not equivalent to SV_Position; the closer + * equivalent is VPOS, which is not declared as a semantic. */ + if (sm1->p.shader_version.type == VKD3D_SHADER_TYPE_VERTEX + && output && semantic->usage == VKD3D_DECL_USAGE_POSITION) + sysval = VKD3D_SHADER_SV_POSITION; + + return add_signature_element(sm1, output, sm1_semantic_names[semantic->usage], + semantic->usage_idx, sysval, reg->idx[0].offset, true, mask); +} + /* Read a parameter token from the input stream, and possibly a relative * addressing token. */ static void shader_sm1_read_param(struct vkd3d_shader_sm1_parser *sm1, @@ -638,6 +879,8 @@ static void shader_sm1_read_semantic(struct vkd3d_shader_sm1_parser *sm1, range = &semantic->resource.range; range->space = 0; range->first = range->last = semantic->resource.reg.reg.idx[0].offset; + + add_signature_element_from_semantic(sm1, semantic); }
static void shader_sm1_read_immconst(struct vkd3d_shader_sm1_parser *sm1, const uint32_t **ptr, @@ -741,6 +984,14 @@ static void shader_sm1_validate_instruction(struct vkd3d_shader_sm1_parser *sm1, } }
+static unsigned int mask_from_swizzle(unsigned int swizzle) +{ + return (1u << vkd3d_swizzle_get_component(swizzle, 0)) + | (1u << vkd3d_swizzle_get_component(swizzle, 1)) + | (1u << vkd3d_swizzle_get_component(swizzle, 2)) + | (1u << vkd3d_swizzle_get_component(swizzle, 3)); +} + static void shader_sm1_read_instruction(struct vkd3d_shader_sm1_parser *sm1, struct vkd3d_shader_instruction *ins) { struct vkd3d_shader_src_param *src_params, *predicate; @@ -829,7 +1080,10 @@ static void shader_sm1_read_instruction(struct vkd3d_shader_sm1_parser *sm1, str { /* Destination token */ if (ins->dst_count) + { shader_sm1_read_dst_param(sm1, &p, dst_param); + add_signature_element_from_register(sm1, &dst_param->reg, false, dst_param->write_mask); + }
/* Predication token */ if (ins->predicate) @@ -837,7 +1091,11 @@ static void shader_sm1_read_instruction(struct vkd3d_shader_sm1_parser *sm1, str
/* Other source tokens */ for (i = 0; i < ins->src_count; ++i) + { shader_sm1_read_src_param(sm1, &p, &src_params[i]); + add_signature_element_from_register(sm1, &src_params[i].reg, + false, mask_from_swizzle(src_params[i].swizzle)); + } }
if (sm1->abort) diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index f1641c1e..5d830e08 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -137,6 +137,8 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_D3DBC_INVALID_OPCODE = 7002, VKD3D_SHADER_ERROR_D3DBC_INVALID_RESOURCE_TYPE = 7003, VKD3D_SHADER_ERROR_D3DBC_OUT_OF_MEMORY = 7004, + VKD3D_SHADER_ERROR_D3DBC_INVALID_REGISTER_INDEX = 7005, + VKD3D_SHADER_ERROR_D3DBC_UNDECLARED_SEMANTIC = 7006,
VKD3D_SHADER_WARNING_D3DBC_IGNORED_INSTRUCTION_FLAGS= 7300, }; @@ -792,6 +794,7 @@ struct signature_element struct shader_signature { struct signature_element *elements; + size_t elements_capacity; unsigned int element_count; };
diff --git a/tests/vkd3d_shader_api.c b/tests/vkd3d_shader_api.c index 318e5068..f9a3f717 100644 --- a/tests/vkd3d_shader_api.c +++ b/tests/vkd3d_shader_api.c @@ -476,6 +476,75 @@ static void test_scan_signatures(void) {"position", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, };
+ static const char vs3_source[] = + "void main(\n" + " in float4 c : position,\n" + " out float4 b : position,\n" + " in float4 a : binormal,\n" + " in float4 d : blendindices,\n" + " inout float4 e : texcoord2,\n" + " inout float4 f : color,\n" + " inout float g : fog,\n" + " inout float h : psize)\n" + "{\n" + " b = a + c + d;\n" + "}"; + + static const struct vkd3d_shader_signature_element vs3_inputs[] = + { + {"POSITION", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + {"BINORMAL", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 1, 0xf, 0xf}, + {"BLENDINDICES", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0xf, 0xf}, + {"TEXCOORD", 2, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 3, 0xf, 0xf}, + {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 4, 0xf, 0xf}, + {"FOG", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 5, 0xf, 0xf}, + {"PSIZE", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 6, 0xf, 0xf}, + }; + + static const struct vkd3d_shader_signature_element vs3_outputs[] = + { + {"POSITION", 0, 0, VKD3D_SHADER_SV_POSITION, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + {"TEXCOORD", 2, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0xf, 0xf}, + {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + {"FOG", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 1, 0x1, 0x1}, + {"PSIZE", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0x1, 0x1}, + }; + + static const char vs4_source[] = + "void main(\n" + " inout float4 c : position,\n" + " inout float4 a : binormal,\n" + " inout float4 d : blendindices,\n" + " inout float4 e : texcoord2,\n" + " inout float4 f : color,\n" + " inout float4 g : fog,\n" + " inout float h : psize)\n" + "{\n" + "}"; + + static const struct vkd3d_shader_signature_element vs4_inputs[] = + { + {"POSITION", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + {"BINORMAL", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 1, 0xf, 0xf}, + {"BLENDINDICES", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0xf, 0xf}, + {"TEXCOORD", 2, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 3, 0xf, 0xf}, + {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 4, 0xf, 0xf}, + {"FOG", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 5, 0xf, 0xf}, + {"PSIZE", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 6, 0xf, 0xf}, + }; + + static const struct vkd3d_shader_signature_element vs4_outputs[] = + { + {"POSITION", 0, 0, VKD3D_SHADER_SV_POSITION, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + {"BINORMAL", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 1, 0xf, 0xf}, + {"BLENDINDICES", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0xf, 0xf}, + {"TEXCOORD", 2, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 3, 0xf, 0xf}, + {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 4, 0xf, 0xf}, + {"FOG", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 5, 0xf, 0xf}, + /* FIXME: This doesn't match native, which always declares and writes all 4 components. */ + {"PSIZE", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 6, 0x1, 0x1}, + }; + static const char ps1_source[] = "void main(\n" " in float2 a : apple,\n" @@ -501,6 +570,72 @@ static void test_scan_signatures(void) {"sv_depth", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, ~0u, 0x1, 0x1}, };
+ static const char ps2_source[] = + "void main(\n" + "in float4 c : color,\n" + "in float4 a : texcoord2,\n" + "out float4 b : color)\n" + "{\n" + "b = a.x + c;\n" + "}"; + + static const struct vkd3d_shader_signature_element ps2_inputs[] = + { + {"TEXCOORD", 2, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0xf, 0xf}, + {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + }; + + static const struct vkd3d_shader_signature_element ps2_outputs[] = + { + {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + }; + + static const char ps3_source[] = + "void main(\n" + "in float4 c : color,\n" + "in float4 a : texcoord2,\n" + "out float4 b : color,\n" + "out float d : depth)\n" + "{\n" + "b = c;\n" + "d = a;\n" + "}"; + + static const struct vkd3d_shader_signature_element ps3_inputs[] = + { + {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + {"TEXCOORD", 2, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0xf, 0xf}, + }; + + static const struct vkd3d_shader_signature_element ps3_outputs[] = + { + {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + {"DEPTH", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0x1, 0x1}, + }; + + static const char ps4_source[] = + "void main(\n" + " in float4 c : color,\n" + " in float4 a : texcoord2,\n" + " out float4 b : color,\n" + " inout float d : depth,\n" + " in float4 e : blendindices,\n" + " in float4 f : vpos,\n" + " in float g : vface)\n" + "{\n" + " b = c + a + e + f + g;\n" + "}"; + + static const struct vkd3d_shader_signature_element ps4_inputs[] = + { + {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + {"TEXCOORD", 2, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 1, 0xf, 0xf}, + {"DEPTH", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0xf, 0xf}, + {"BLENDINDICES", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 3, 0xf, 0xf}, + {"VPOS", 0, 0, VKD3D_SHADER_SV_POSITION, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + {"VFACE", 0, 0, VKD3D_SHADER_SV_IS_FRONT_FACE, VKD3D_SHADER_COMPONENT_FLOAT, 1, 0x1, 0x1}, + }; + static const char cs1_source[] = "[numthreads(1, 1, 1)]\n" "void main(\n" @@ -513,6 +648,7 @@ static void test_scan_signatures(void) static const struct { const char *source; + bool sm4; const char *profile; const struct vkd3d_shader_signature_element *inputs; size_t input_count; @@ -523,10 +659,16 @@ static void test_scan_signatures(void) } tests[] = { - {vs1_source, "vs_4_0", vs1_inputs, ARRAY_SIZE(vs1_inputs), vs1_outputs, ARRAY_SIZE(vs1_outputs)}, - {vs2_source, "vs_4_0", vs2_inputs, ARRAY_SIZE(vs2_inputs), vs2_outputs, ARRAY_SIZE(vs2_outputs)}, - {ps1_source, "ps_4_0", ps1_inputs, ARRAY_SIZE(ps1_inputs), ps1_outputs, ARRAY_SIZE(ps1_outputs)}, - {cs1_source, "cs_5_0", NULL, 0, NULL, 0}, + {vs1_source, true, "vs_4_0", vs1_inputs, ARRAY_SIZE(vs1_inputs), vs1_outputs, ARRAY_SIZE(vs1_outputs)}, + {vs2_source, true, "vs_4_0", vs2_inputs, ARRAY_SIZE(vs2_inputs), vs2_outputs, ARRAY_SIZE(vs2_outputs)}, + {vs3_source, false, "vs_1_1", vs3_inputs, ARRAY_SIZE(vs3_inputs), vs3_outputs, ARRAY_SIZE(vs3_outputs)}, + {vs3_source, false, "vs_2_0", vs3_inputs, ARRAY_SIZE(vs3_inputs), vs3_outputs, ARRAY_SIZE(vs3_outputs)}, + {vs4_source, false, "vs_3_0", vs4_inputs, ARRAY_SIZE(vs4_inputs), vs4_outputs, ARRAY_SIZE(vs4_outputs)}, + {ps1_source, true, "ps_4_0", ps1_inputs, ARRAY_SIZE(ps1_inputs), ps1_outputs, ARRAY_SIZE(ps1_outputs)}, + {ps2_source, false, "ps_1_1", ps2_inputs, ARRAY_SIZE(ps2_inputs), ps2_outputs, ARRAY_SIZE(ps2_outputs)}, + {ps3_source, false, "ps_2_0", ps3_inputs, ARRAY_SIZE(ps3_inputs), ps3_outputs, ARRAY_SIZE(ps3_outputs)}, + {ps4_source, false, "ps_3_0", ps4_inputs, ARRAY_SIZE(ps4_inputs), ps3_outputs, ARRAY_SIZE(ps3_outputs)}, + {cs1_source, true, "cs_5_0", NULL, 0, NULL, 0}, };
for (i = 0; i < ARRAY_SIZE(tests); ++i) @@ -536,7 +678,7 @@ static void test_scan_signatures(void) compile_info.source.code = tests[i].source; compile_info.source.size = strlen(tests[i].source); compile_info.source_type = VKD3D_SHADER_SOURCE_HLSL; - compile_info.target_type = VKD3D_SHADER_TARGET_DXBC_TPF; + compile_info.target_type = tests[i].sm4 ? VKD3D_SHADER_TARGET_DXBC_TPF : VKD3D_SHADER_TARGET_D3D_BYTECODE; compile_info.log_level = VKD3D_SHADER_LOG_INFO;
compile_info.next = &hlsl_info; @@ -545,7 +687,7 @@ static void test_scan_signatures(void) rc = vkd3d_shader_compile(&compile_info, &dxbc, NULL); ok(rc == VKD3D_OK, "Got unexpected error code %d.\n", rc);
- compile_info.source_type = VKD3D_SHADER_SOURCE_DXBC_TPF; + compile_info.source_type = tests[i].sm4 ? VKD3D_SHADER_SOURCE_DXBC_TPF : VKD3D_SHADER_SOURCE_D3D_BYTECODE; compile_info.source = dxbc;
compile_info.next = &signature_info;
On Tue May 16 16:38:25 2023 +0000, Zebediah Figura wrote:
True. On the other hand, it's zeroed for d3dbc, and we probably want to zero it everywhere else for consistency...
Done in v3; we always zero that struct in vkd3d_shader_compile() and vkd3d_shader_scan() now.
On Wed May 17 11:42:25 2023 +0000, Giovanni Mascellani wrote:
Yeah, "legacy" is probably not ideal. The point is not to communicate that it shouldn't be used, rather than to spare the user from thinking "Why do they have two interfaces for doing two similar things, am I missing anything?".
Changed in v3. For convenience, here's the wording I used:
``` + * This function only parses DXBC shaders, and only retrieves the input + * signature. To retrieve signatures from other shader types, or other signature + * types, use vkd3d_shader_scan() and struct vkd3d_shader_scan_signature_info. + * This function returns the same input signature that is returned in + * struct vkd3d_shader_scan_signature_info. ```
Giovanni Mascellani (@giomasce) commented about include/vkd3d_shader.h:
- For shader model 3 pixel position inputs (i.e. VPOS, but not POSITION),
it is set to VKD3D_SHADER_SV_POSITION.
- For shader model 3 face inputs (i.e. VFACE), it is set to
VKD3D_SHADER_SV_IS_FRONT_FACE.
- For all other elements, it is set to VKD3D_SHADER_SV_NONE.
- The \ref vkd3d_shader_signature_element.component_type field is always set
- to VKD3D_SHADER_COMPONENT_FLOAT.
- The \ref vkd3d_shader_signature_element.register_index field is set to the
- bytecode register index.
- Note that for shader model 1 and 2 shaders (excepting vertex shader
- inputs), the register index of colour and texture coordinate registers will
- be equal to the usage index, and hence may not be unique.
- The \ref vkd3d_shader_signature_element.mask field is set to the mask given
- in the DCL instruction, if one is present. If there is no DCL instruction
- for this semantic, it is set to the same value as the \ref used_mask field.
- For the scalar registers vFace, oDepth, oFog, and oPts, it is always 1.
It seems that the program does something slightly different: it ORs together all the masks, including the one in the DCL. Which probably usually coincides with what you say, given that hopefully the DCL mask is a subset of any other, but I guess there might be exceptions.
I don't know if it makes sense to be precise about this kind of things, so feel free to ignore this. I just noticed this fact, so I wanted to mention it.
This merge request was approved by Giovanni Mascellani.
As usual if have pretty zero working experience with SM1, so I won't judge the merit. At least on the level of documentation and programming it all makes sense to me, except for a little comment.
+ /** + * The structure is a vkd3d_shader_scan_signature_info structure. + * \since 1.x + */
That should be "\since 1.8". There are a few other instances of this as well.
+/** + * A chained structure containing signatures scanned from a DXBC shader. + * + * All members (except for \ref type and \ref next) are output-only. + * + * This structure is passed to vkd3d_shader_scan() and extends + * vkd3d_shader_compile_info. + * + * Members of this structure are allocated by vkd3d-shader and should be freed + * with vkd3d_shader_free_scan_signature_info() when no longer needed. + * + * \since 1.x + */ +struct vkd3d_shader_scan_signature_info +{ + /** Must be set to VKD3D_SHADER_STRUCTURE_TYPE_SCAN_SIGNATURE_INFO. */ + enum vkd3d_shader_structure_type type; + /** Optional pointer to a structure containing further parameters. */ + const void *next; + + /** + * The parsed input signature embedded in a DXBC shader. + * The structure is zeroed for any other type of shader. + * + * The signature may contain pointers into the input shader, and should + * only be accessed while the input shader remains valid. + */ + struct vkd3d_shader_signature input; + + /** + * The parsed output signature embedded in a DXBC shader. + * The structure is zeroed for any other type of shader. + * + * The signature may contain pointers into the input shader, and should + * only be accessed while the input shader remains valid. + */ + struct vkd3d_shader_signature output; + + /** + * The parsed patch constant signature embedded in a DXBC hull shader. + * The structure is zeroed for any other type of shader. + * + * The signature may contain pointers into the input shader, and should + * only be accessed while the input shader remains valid. + */ + struct vkd3d_shader_signature patch_constant; +};
The text here ties this fairly strongly to the input format, as well as the specifics of DXBC containers. I'm not going to object too much, but I'm not sure that's entirely warranted; I could imagine the the target format/API making a difference for what we return here, as well as compilation options. I could also imagine e.g. compiling HLSL to GLSL, and still wanting information about the input/output interface. Perhaps notably, the API tests here use HLSL as actual source.
The text here ties this fairly strongly to the input format, as well as the specifics of DXBC containers. I'm not going to object too much, but I'm not sure that's entirely warranted; I could imagine the the target format/API making a difference for what we return here, as well as compilation options. I could also imagine e.g. compiling HLSL to GLSL, and still wanting information about the input/output interface. Perhaps notably, the API tests here use HLSL as actual source.
Is there a notion of shader signature which is at the same time sufficiently general so that it applies to all (or at least most) shader formats and sufficiently specific so that it's actually usable for something?
The text here ties this fairly strongly to the input format, as well as the specifics of DXBC containers. I'm not going to object too much, but I'm not sure that's entirely warranted; I could imagine the the target format/API making a difference for what we return here, as well as compilation options. I could also imagine e.g. compiling HLSL to GLSL, and still wanting information about the input/output interface. Perhaps notably, the API tests here use HLSL as actual source.
Is there a notion of shader signature which is at the same time sufficiently general so that it applies to all (or at least most) shader formats and sufficiently specific so that it's actually usable for something?
I think the current vkd3d_shader_signature structure should work for most purposes, although an argument could be made for extending it with some of the additional DXIL and SPIR-V features.
Taking a step back though, signatures provide information about the interface between shaders and their environment:
- For a vertex shader input signature, that's mainly the mapping of shader input registers to vertex attributes in the target API (i.e., OpenGL/Vulkan/Direct3D/Metal). - For a vertex shader output signature, that's the mapping of shader output registers to the interstage interface with the next shader. - For a root signature, that's the mapping of shader resources (CBV/SRV/UAV) and samplers to the corresponding bind points in the target API.
That means there are broadly two kinds of usage for this information:
- Setting up bindings (attributes, resources, samplers, etc.) in the target environment. - Matching up the interstage interface during shader translation.
Due to differences in target APIs, it can sometimes be useful to specify the interface from the point of view of the target environment as well. For example, this is the case when translating Direct3D 11 shaders to SPIR-V/Vulkan; Direct3D 11 has fixed API bind points for resources, while Vulkan has the more flexible descriptor sets/layouts.
The approach taken for descriptor bindings is that the vkd3d_shader_scan_descriptor_info structure returns information about the descriptors used by the shader, while the vkd3d_shader_interface_info structure provides information about how to map these to the target environment. (And by contrast, vkd3d_shader_parse_root_signature() and vkd3d_shader_parse_input_signature() just parse the raw data.)
There's probably an argument to be made for not lumping input attribute binding information and interstage interface information together as well...
That should be "\since 1.8". There are a few other instances of this as well.
Of course this was intentional, but yes, I'll change it now.
The text here ties this fairly strongly to the input format, as well as the specifics of DXBC containers. I'm not going to object too much, but I'm not sure that's entirely warranted; I could imagine the the target format/API making a difference for what we return here, as well as compilation options. I could also imagine e.g. compiling HLSL to GLSL, and still wanting information about the input/output interface. Perhaps notably, the API tests here use HLSL as actual source.
I'm not really sure I understand this comment.
In the first place, yes, this is tied to DXBC, because vkd3d_shader_signature is very much based on DXBC. You can come up with something similar for other formats—as done in 2/2—but it requires inventing some degree of mapping (even for d3dbc it's not clear how semantic names or masks get mapped). As an API consumer, I'd want it to be very clear what i/o variables are included in the shader or not (consider DXBC isn't exactly consistent about this: from an HLSL perspective, it seems arbitrary that pixel shader SV_Position is included in the signature but vertex shader SV_VertexID isn't) and how that mapping is done.
In the second place, I don't understand how the target format affects anything. This is a scanning option, and it's currently implemented for vkd3d_shader_compile() only inasmuch as vkd3d_shader_compile() implicitly scans the source. If we want it to do something different, I think we need to work that out right now and document it.
I think what you're trying to say is that we should put information about the GLSL (or Vulkan) binding here, but that seems to me like fundamentally a contradictory model to what we already have. Descriptor info doesn't work this way. If we want to provide feedback about the default mapping of varyings to the target format, and we can't rely on storing reflection data in that format (like with hlsl -> dxbc translation) I think we need to do that in some separate structure.
The text here ties this fairly strongly to the input format, as well as the specifics of DXBC containers. I'm not going to object too much, but I'm not sure that's entirely warranted; I could imagine the the target format/API making a difference for what we return here, as well as compilation options. I could also imagine e.g. compiling HLSL to GLSL, and still wanting information about the input/output interface. Perhaps notably, the API tests here use HLSL as actual source.
I'm not really sure I understand this comment.
In the first place, yes, this is tied to DXBC, because vkd3d_shader_signature is very much based on DXBC. You can come up with something similar for other formats—as done in 2/2—but it requires inventing some degree of mapping (even for d3dbc it's not clear how semantic names or masks get mapped).
Sure, but I'm not sure that's necessarily an argument for tying this to DXBC. After this series, the only unsupported source format is HLSL, and I think it would be straightforward enough to support there as well. In terms of potential future formats, we'd definitely want this to support DXIL, and likewise for something like d3d-asm or dxil-asm.
I don't expect we'll ever support e.g. GLSL as source format, but note that the mapping there would be straightforward as well.
On the other hand, if we really wanted to do something specific to DXBC, we'd only need to expose some variant of shader_parse_signature(); we already have the API for the caller to extract the ISGN/OSGN/PCSG DXBC sections and their variants, and there would be no need to parse the shader's source in that case.
As an API consumer, I'd want it to be very clear what i/o variables are included in the shader or not (consider DXBC isn't exactly consistent about this: from an HLSL perspective, it seems arbitrary that pixel shader SV_Position is included in the signature but vertex shader SV_VertexID isn't) and how that mapping is done.
Sure, although I'd argue that what you actually want to know as a user of this API is either what setup you'll need to do in the target API to use this shader, or what interface the shader for the next stage needs to have. Note that the former may somewhat depend on the target format/API. For example, for SPIR-V/Vulkan the "rasterizer" (VKD3DSPR_RASTERIZER) register needs to be supplied explicitly, while that's implicit for TPF/d3d11.
In the second place, I don't understand how the target format affects anything. This is a scanning option, and it's currently implemented for vkd3d_shader_compile() only inasmuch as vkd3d_shader_compile() implicitly scans the source. If we want it to do something different, I think we need to work that out right now and document it.
Consider e.g. the earlier BLENDINDICES discussion; when targetting an API without _USCALED/_SSCALED formats, we may want to consider BLENDINDICES as UINT. I don't necessarily have a lot of good examples here, but I wouldn't want to rule the scenario out up-front either.
I think what you're trying to say is that we should put information about the GLSL (or Vulkan) binding here, but that seems to me like fundamentally a contradictory model to what we already have. Descriptor info doesn't work this way. If we want to provide feedback about the default mapping of varyings to the target format, and we can't rely on storing reflection data in that format (like with hlsl -> dxbc translation) I think we need to do that in some separate structure.
No, I think that's a misunderstanding. I'm mostly just suggesting that I think it would be fine for the documentation to be phrased in terms of something along the lines of "a description of the shader's input/output parameters", instead of something along the lines of "this thing we extracted from a DXBC blob, except when we didn't".
I don't think there's an issue with the vkd3d_shader_scan_signature_info structure itself, and I could live with the current text if needed.
The text here ties this fairly strongly to the input format, as well as the specifics of DXBC containers. I'm not going to object too much, but I'm not sure that's entirely warranted; I could imagine the the target format/API making a difference for what we return here, as well as compilation options. I could also imagine e.g. compiling HLSL to GLSL, and still wanting information about the input/output interface. Perhaps notably, the API tests here use HLSL as actual source.
I'm not really sure I understand this comment.
In the first place, yes, this is tied to DXBC, because vkd3d_shader_signature is very much based on DXBC. You can come up with something similar for other formats—as done in 2/2—but it requires inventing some degree of mapping (even for d3dbc it's not clear how semantic names or masks get mapped).
Sure, but I'm not sure that's necessarily an argument for tying this to DXBC. After this series, the only unsupported source format is HLSL, and I think it would be straightforward enough to support there as well. In terms of potential future formats, we'd definitely want this to support DXIL, and likewise for something like d3d-asm or dxil-asm.
It's not impossible, but I'm not sure it's trivial and obvious either. What goes in the register_index field? (Zero?) Should SV_ThreadID be mapped?
On the other hand, if we really wanted to do something specific to DXBC, we'd only need to expose some variant of shader_parse_signature(); we already have the API for the caller to extract the ISGN/OSGN/PCSG DXBC sections and their variants, and there would be no need to parse the shader's source in that case.
It's not that I think this API needs to be specific to DXBC—far from it, the whole impetus for that patch is 2/2 which extends it to sm1—but I don't think we can just support every format right off the bat without nontrivial API design. I don't exactly want that to hold up sm1 support if there's not a good reason.
As an API consumer, I'd want it to be very clear what i/o variables are included in the shader or not (consider DXBC isn't exactly consistent about this: from an HLSL perspective, it seems arbitrary that pixel shader SV_Position is included in the signature but vertex shader SV_VertexID isn't) and how that mapping is done.
Sure, although I'd argue that what you actually want to know as a user of this API is either what setup you'll need to do in the target API to use this shader, or what interface the shader for the next stage needs to have. Note that the former may somewhat depend on the target format/API. For example, for SPIR-V/Vulkan the "rasterizer" (VKD3DSPR_RASTERIZER) register needs to be supplied explicitly, while that's implicit for TPF/d3d11.
In theory, sure. In practice, the information DXBC exposes doesn't really align with that. For one thing, I misremembered: SV_VertexID actually does go in the input signature, despite not actually hooking up to the target API in any way. On the other hand, SV_DispatchThreadID doesn't.
In any case, this is something I really want to be spelled out as an API consumer. There is no greater crime than vagueness.
In the second place, I don't understand how the target format affects anything. This is a scanning option, and it's currently implemented for vkd3d_shader_compile() only inasmuch as vkd3d_shader_compile() implicitly scans the source. If we want it to do something different, I think we need to work that out right now and document it.
Consider e.g. the earlier BLENDINDICES discussion; when targetting an API without _USCALED/_SSCALED formats, we may want to consider BLENDINDICES as UINT. I don't necessarily have a lot of good examples here, but I wouldn't want to rule the scenario out up-front either.
I'm not sure this kind of thing really belongs in the scan information. Honestly it just feels ugly to me to imply that scanning can depend on anything other than the shader itself.
I think what you're trying to say is that we should put information about the GLSL (or Vulkan) binding here, but that seems to me like fundamentally a contradictory model to what we already have. Descriptor info doesn't work this way. If we want to provide feedback about the default mapping of varyings to the target format, and we can't rely on storing reflection data in that format (like with hlsl -> dxbc translation) I think we need to do that in some separate structure.
No, I think that's a misunderstanding. I'm mostly just suggesting that I think it would be fine for the documentation to be phrased in terms of something along the lines of "a description of the shader's input/output parameters", instead of something along the lines of "this thing we extracted from a DXBC blob, except when we didn't".
I don't think there's an issue with the vkd3d_shader_scan_signature_info structure itself, and I could live with the current text if needed.
Sure, I think I understand not wanting to marry the text to a specific output format. The problem is that anything less than that just isn't specific enough. I could probably reword it so that the *lede* isn't married to the output format, but at some point I think we really do need to specify how the signature was parsed or generated.
Sure, but I'm not sure that's necessarily an argument for tying this to DXBC. After this series, the only unsupported source format is HLSL, and I think it would be straightforward enough to support there as well. In terms of potential future formats, we'd definitely want this to support DXIL, and likewise for something like d3d-asm or dxil-asm.
It's not impossible, but I'm not sure it's trivial and obvious either. What goes in the register_index field? (Zero?) Should SV_ThreadID be mapped?
Well, we'd want it to return the same information as we'd get when first compiling to TPF or d3dbc and then scanning the resulting shader. (And the straightforward though perhaps not entirely optimal way to implement HLSL scans would of course be to do just that.) That does imply that e.g. the HLSL target profile may make a difference.
Sure, although I'd argue that what you actually want to know as a user of this API is either what setup you'll need to do in the target API to use this shader, or what interface the shader for the next stage needs to have. Note that the former may somewhat depend on the target format/API. For example, for SPIR-V/Vulkan the "rasterizer" (VKD3DSPR_RASTERIZER) register needs to be supplied explicitly, while that's implicit for TPF/d3d11.
In theory, sure. In practice, the information DXBC exposes doesn't really align with that. For one thing, I misremembered: SV_VertexID actually does go in the input signature, despite not actually hooking up to the target API in any way. On the other hand, SV_DispatchThreadID doesn't.
Right, but if anything, that seems to argue against just returning the parsed input/output signatures contained in the DXBC.
In any case, this is something I really want to be spelled out as an API consumer. There is no greater crime than vagueness.
I don't know; I think that over the years I've come to appreciate the fact that ambiguity is the glue that holds many things together. :)
Consider e.g. the earlier BLENDINDICES discussion; when targetting an API without _USCALED/_SSCALED formats, we may want to consider BLENDINDICES as UINT. I don't necessarily have a lot of good examples here, but I wouldn't want to rule the scenario out up-front either.
I'm not sure this kind of thing really belongs in the scan information. Honestly it just feels ugly to me to imply that scanning can depend on anything other than the shader itself.
I tend to think of vkd3d_shader_scan() as providing the information needed to use the shader in the target environment. (And note that the function fairly deliberately takes a vkd3d_shader_compile_info structure as its input.) For modern formats like TPF and DXIL that can largely be derived purely from the original shader source; for something like d3dbc there is a lot more implicit state (e.g., fixed-function fog state) provided by the environment, as I'm sure you've already encountered.
There may be value in providing information purely about the original shader in its original intended environment, and we could potentially support that in vkd3d_shader_scan() by supporting VKD3D_SHADER_TARGET_NONE as target, but it's not quite how vkd3d_shader_scan() is currently intended.
No, I think that's a misunderstanding. I'm mostly just suggesting that I think it would be fine for the documentation to be phrased in terms of something along the lines of "a description of the shader's input/output parameters", instead of something along the lines of "this thing we extracted from a DXBC blob, except when we didn't".
I don't think there's an issue with the vkd3d_shader_scan_signature_info structure itself, and I could live with the current text if needed.
Sure, I think I understand not wanting to marry the text to a specific output format. The problem is that anything less than that just isn't specific enough. I could probably reword it so that the *lede* isn't married to the output format, but at some point I think we really do need to specify how the signature was parsed or generated.
I don't necessarily have an issue with documenting that, although it does feel like documenting implementation details instead of documenting what's provided by the API.
Well, we'd want it to return the same information as we'd get when first compiling to TPF or d3dbc and then scanning the resulting shader. (And the straightforward though perhaps not entirely optimal way to implement HLSL scans would of course be to do just that.) That does imply that e.g. the HLSL target profile may make a difference.
This probably fundamentally comes down to the points raised below, but this especially doesn't make sense to me. It doesn't seem consistent that we'd return information about the DXBC shader for a DXBC shader, but information *also* about the DXBC shader for an HLSL shader. It's also completely redundant; if someone really wants information about the DXBC shader, she can just compile to DXBC first.
I don't know; I think that over the years I've come to appreciate the fact that ambiguity is the glue that holds many things together. :)
This may end up being a sticking point, if not here than in the future. I don't understand this viewpoint at all. Having consumed a dozen APIs by this point this is consistently the thing that distinguishes the pleasant ones (Vulkan, X11) from the unpleasant ones (GStreamer, all of Win32). Finding out there's no way to perform some task is frustrating, but wasting time trying to figure out whether I can perform some task—and then later worrying about whether the approach taken is safe—is worse. Or wasting time trying to guess whose bug something is and how it should be fixed.
Vagueness makes things nice for the implementers, but it has always made my life unpleasant as a consumer. I intend to argue as hard as I can for putting the consumer first in any API I design. With all of that said, I certainly haven't been programming as long as you, and if it can be communicated I'd definitely like to understand why vagueness can be a good thing (or inevitable, or whatever it is).
I tend to think of vkd3d_shader_scan() as providing the information needed to use the shader in the target environment. (And note that the function fairly deliberately takes a vkd3d_shader_compile_info structure as its input.) For modern formats like TPF and DXIL that can largely be derived purely from the original shader source; for something like d3dbc there is a lot more implicit state (e.g., fixed-function fog state) provided by the environment, as I'm sure you've already encountered.
There may be value in providing information purely about the original shader in its original intended environment, and we could potentially support that in vkd3d_shader_scan() by supporting VKD3D_SHADER_TARGET_NONE as target, but it's not quite how vkd3d_shader_scan() is currently intended.
This doesn't seem intuitive to me at all, and I don't think it's how API users are going to interpret vkd3d_shader_scan(), despite the function signature. I think this also makes vkd3d-shader more confusing, if we're introducing behaviour that's implicit on the target type or environment. It's more to keep track of for the implementer and the user. *Especially* if we aren't going to explicitly document these "implementation details"—that's a sure way to confuse a user about "why is vkd3d-shader returning different data between these two cases?"
And it seems like scope creep. I don't want to argue about this point because I don't think the scope can be rigidly defined, but I think trying to change information about the shader we return *based on* the target environment seems to me like it's outside of the scope of a shader manipulation library. Let the API user change BLENDINDICES if they want to.
Well, we'd want it to return the same information as we'd get when first compiling to TPF or d3dbc and then scanning the resulting shader. (And the straightforward though perhaps not entirely optimal way to implement HLSL scans would of course be to do just that.) That does imply that e.g. the HLSL target profile may make a difference.
This probably fundamentally comes down to the points raised below, but this especially doesn't make sense to me. It doesn't seem consistent that we'd return information about the DXBC shader for a DXBC shader, but information *also* about the DXBC shader for an HLSL shader. It's also completely redundant; if someone really wants information about the DXBC shader, she can just compile to DXBC first.
Perhaps the discussion is getting a bit too abstract, and maybe that's a point in favour of your point of view. Regardless, the way I look at it is that a shader is a set of operations to be performed by a GPU (which may in fact end up being a CPU, but anyway...), and both the source and target shader are different representations of the same set of operations; at the same time, that set of operations isn't necessarily fully defined by the original source code, and that's where the additional source/target info comes in. The implication would be that [HLSL->SPIR-V] should return the same information as [HLSL->TPF->SPIR-V] given the same/equivalent compilation options. I.e., we are just as much returning information about the target SPIR-V as we're returning information about the source HLSL or TPF.
I don't know; I think that over the years I've come to appreciate the fact that ambiguity is the glue that holds many things together. :)
This may end up being a sticking point, if not here than in the future. I don't understand this viewpoint at all. Having consumed a dozen APIs by this point this is consistently the thing that distinguishes the pleasant ones (Vulkan, X11) from the unpleasant ones (GStreamer, all of Win32). Finding out there's no way to perform some task is frustrating, but wasting time trying to figure out whether I can perform some task—and then later worrying about whether the approach taken is safe—is worse. Or wasting time trying to guess whose bug something is and how it should be fixed.
Vagueness makes things nice for the implementers, but it has always made my life unpleasant as a consumer. I intend to argue as hard as I can for putting the consumer first in any API I design. With all of that said, I certainly haven't been programming as long as you, and if it can be communicated I'd definitely like to understand why vagueness can be a good thing (or inevitable, or whatever it is).
Sorry, I didn't mean to imply being vague in the API documentation was desirable or appropriate here; I was just making a rather general comment in reply to a fairly broad statement. :)
To be clear, I think accurate, unambiguous, complete API documentation is an immense asset.
I tend to think of vkd3d_shader_scan() as providing the information needed to use the shader in the target environment. (And note that the function fairly deliberately takes a vkd3d_shader_compile_info structure as its input.) For modern formats like TPF and DXIL that can largely be derived purely from the original shader source; for something like d3dbc there is a lot more implicit state (e.g., fixed-function fog state) provided by the environment, as I'm sure you've already encountered.
There may be value in providing information purely about the original shader in its original intended environment, and we could potentially support that in vkd3d_shader_scan() by supporting VKD3D_SHADER_TARGET_NONE as target, but it's not quite how vkd3d_shader_scan() is currently intended.
This doesn't seem intuitive to me at all, and I don't think it's how API users are going to interpret vkd3d_shader_scan(), despite the function signature. I think this also makes vkd3d-shader more confusing, if we're introducing behaviour that's implicit on the target type or environment. It's more to keep track of for the implementer and the user. *Especially* if we aren't going to explicitly document these "implementation details"—that's a sure way to confuse a user about "why is vkd3d-shader returning different data between these two cases?"
I fear you may be right, if perhaps largely by the fact that we're having this discussion.
And it seems like scope creep. I don't want to argue about this point because I don't think the scope can be rigidly defined, but I think trying to change information about the shader we return *based on* the target environment seems to me like it's outside of the scope of a shader manipulation library. Let the API user change BLENDINDICES if they want to.
There are implications to that though.
Suppose we invent this imaginary graphics API; we could call it the "Fairly Interactive Graphics System", because figs are delicious. We'll make it accept SPIR-V—because, why not?—and add VKD3D_SHADER_SPIRV_ENVIRONMENT_FIGS_1_0 to vkd3d-shader. Due to an unfortunate oversight, FIGS 1.0 doesn't support scaled floating-point vertex attribute formats, so we decide that it would be most appropriate for vkd3d-shader to translate d3dbc BLENDINDICES inputs to pure integer inputs when targetting FIGS 1.0. So far, so good.
Now we're going to port an application to the brand new FIGS 1.0 API, and it uses these d3dbc shaders. Since vkd3d-shader supports translating them to SPIR-V we figure we might as well use them, instead of spending a bunch of time writing brand new shaders using spirv-as. ;) So we use vkd3d_shader_scan() to retrieve the information needed to setup our resource and attribute bindings, and we use vkd3d_shader_compile() to generate a corresponding SPIR-V shader. We run our application, and... all our character models are weird. [Debugging session omitted for brevity.] As it turns out, we should have ignored vkd3d_shader_scan() telling us BLENDINDICES receives floating-point data and sent pure integers instead.
So we go through the data we receive from vkd3d_shader_scan(), fix it up, and now everything works well enough to ship the application. :) That's not an entirely satisfying solution though, because now we're relying on vkd3d-shader internals in our application. The vkd3d-shader API documentation doesn't promise this is something we can rely on, but hopefully they'll remember not to break this... right?
So where did things go wrong?
**Obviously, we should have called vkd3d_shader_scan() on the resulting SPIR-V instead of the d3dbc source.** Possibly, except that vkd3d-shader doesn't support SPIR-V as a source format. **So vkd3d-shader should just add that, right?** Sure, it could. But that then implies it should accept any target format as source format as well. And while parsing SPIR-V is relatively straightforward, that's not necessarily true for something like GLSL or MSL.
**Obviously, we should have used SPIRV-Reflect instead of vkd3d-shader to get this information.** Sure, that works. In that case we're adding an extra dependency just to workaround a relatively minor issues though. And what is the point of providing vkd3d_shader_scan() then? If we were going to run the original source shaders we wouldn't have needed vkd3d-shader to translate them to SPIR-V...
**Obviously, we should have used our own shader I/O metadata embedded in our application's assets.** Yeah, that works too. Unfortunately the metadata was lost in an accident several years ago, along with the Cg source for the shaders. Also, what is the point of vkd3d_shader_scan() then?
**Obviously, vkd3d-shader should have left BLENDINDICES alone.** Well, that means potentially converting vertex data at runtime in the application. A bunch of extra code, and likely some performance impact. **How about making it a vkd3d_shader_compile_option instead of changing behaviour based on VKD3D_SHADER_SPIRV_ENVIRONMENT_FIGS_1_0?** Maybe that's better, but it doesn't change the fundamental problem; we're still not going to get the information we want from vkd3d_shader_scan().
**Clearly we should have written new shaders using spirv-as instead of using vkd3d-shader.** That would be a bit of an unfortunate conclusion...
Sorry for the delay—I sensed I might be getting tunnel vision, and wanted to come back to it with a possibly clearer head.
There is a conflict between "scanning is an operation that should not depend on any external data"—which I still think is a fundamentally sensible position—and "scanning should make a best guess as to how the bindings will actually be used"—which also makes sense, or I wouldn't have proposed it in the first place.
Or, perhaps more saliently, "scanning should be consistent with compiling".
I still can't shake the feeling that it feels like scope creep, but I also kind of understand where the idea is coming from.
So here's my proposal:
* Allow passing VKD3D_SHADER_TARGET_NONE to vkd3d_shader_scan(). This satisfies the API consumer in me that is perpetually baffled by the fact that vkd3d_shader_scan() accepts a target type. If we receive NONE, we return whatever is fundamentally truest about the source type. In this case I think that means BLENDINDICES is FLOAT. This may end up also being the case if the target type is something nonsensical like SPIRV_TEXT or D3D_ASM...
* We go ahead and automatically map BLENDINDICES to UINT if and only if compiling to SPIRV (or GLSL) and the environment is GL. [Or... should we do it for Vulkan too? The scaled-float formats exist, but are they as well-supported? Plus that'll require a pointless, albeit probably not destructive, round trip in the Vulkan driver.]
* If no environment is specified (environment is NONE or spirv_target_info is missing), we assume Vulkan. This is basically called out in the documentation anyway, but I figured I'd mention it for completeness sake.
* Document that when the target type isn't NONE, we'll return a format that reflects how that target type will interpret the data. I think if we do this we don't actually have to say what that target type will be and under which conditions (or keep it constant across library versions?), although I certainly won't argue against doing that anyway.
There is a conflict between "scanning is an operation that should not depend on any external data"—which I still think is a fundamentally sensible position—and "scanning should make a best guess as to how the bindings will actually be used"—which also makes sense, or I wouldn't have proposed it in the first place.
Sure; that apparent conflict largely depends on the definition of "external" above though. I think it would be reasonable to consider the source and target information to be part of the abstract shader, as opposed to e.g. data contained in resources bound at runtime.
Or, perhaps more saliently, "scanning should be consistent with compiling".
Right.
- Allow passing VKD3D_SHADER_TARGET_NONE to vkd3d_shader_scan(). This satisfies the API consumer in me that is perpetually baffled by the fact that vkd3d_shader_scan() accepts a target type. If we receive NONE, we return whatever is fundamentally truest about the source type. In this case I think that means BLENDINDICES is FLOAT. This may end up also being the case if the target type is something nonsensical like SPIRV_TEXT or D3D_ASM...
Sure, that's fine with me. It's perhaps worth pointing out though that this may imply e.g. returning "VKD3D_SHADER_RESOURCE_NONE" as "resource_type" in "vkd3d_shader_descriptor_info" for D3D shader model 1 shaders if we're not going to consider source information either.
- We go ahead and automatically map BLENDINDICES to UINT if and only if compiling to SPIRV (or GLSL) and the environment is GL. [Or... should we do it for Vulkan too? The scaled-float formats exist, but are they as well-supported? Plus that'll require a pointless, albeit probably not destructive, round trip in the Vulkan driver.]
I don't think we need that. OpenGL (still) supports scaled-float formats. It could hypothetically be an issue when targetting d3d10+, e.g. when doing something like d3dbc->tpf/dxil. (Which makes me wonder... is this an issue ANGLE would have with translating GL to D3D?)
- If no environment is specified (environment is NONE or spirv_target_info is missing), we assume Vulkan. This is basically called out in the documentation anyway, but I figured I'd mention it for completeness sake.
Right, and in general the documentation for the source/target information structures should spell out the values used when those structures aren't present.
- Document that when the target type isn't NONE, we'll return a format that reflects how that target type will interpret the data. I think if we do this we don't actually have to say what that target type will be and under which conditions (or keep it constant across library versions?), although I certainly won't argue against doing that anyway.
Right, this is the main thing I've tried to argue for.
- Allow passing VKD3D_SHADER_TARGET_NONE to vkd3d_shader_scan(). This satisfies the API consumer in me that is perpetually baffled by the fact that vkd3d_shader_scan() accepts a target type. If we receive NONE, we return whatever is fundamentally truest about the source type. In this case I think that means BLENDINDICES is FLOAT. This may end up also being the case if the target type is something nonsensical like SPIRV_TEXT or D3D_ASM...
Sure, that's fine with me. It's perhaps worth pointing out though that this may imply e.g. returning "VKD3D_SHADER_RESOURCE_NONE" as "resource_type" in "vkd3d_shader_descriptor_info" for D3D shader model 1 shaders if we're not going to consider source information either.
That seems reasonable to me.
- We go ahead and automatically map BLENDINDICES to UINT if and only if compiling to SPIRV (or GLSL) and the environment is GL. [Or... should we do it for Vulkan too? The scaled-float formats exist, but are they as well-supported? Plus that'll require a pointless, albeit probably not destructive, round trip in the Vulkan driver.]
I don't think we need that. OpenGL (still) supports scaled-float formats. It could hypothetically be an issue when targetting d3d10+, e.g. when doing something like d3dbc->tpf/dxil. (Which makes me wonder... is this an issue ANGLE would have with translating GL to D3D?)
Oh, I wasn't aware that GL had scaled-float formats. Reading the documentation... it appears that's what happens if you use glVertexAttribPointer() [not glVertexAttribIPointer()] with GL_INT but leave "normalized" as FALSE.
-
Giovanni Mascellani (＠giomasce) -
Henri Verbeet (＠hverbeet)
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)