This is probably the most ugly and controversial bit of API. I don't really know if this is the right approach to solving this.
sm4 registers match by register index, such that shaders can mostly be compiled in isolation. sm1 does not—registers may be specified in any order in the vertex and pixel shaders, and will be matched by usage and usage index.
By itself this is not much of a problem. Where it gets hairy is that we want to do some degree of caching, as well as pre-compilation, and avoid recompiling either shader every time it's matched with a new one.
Wine currently deals with this problem, for GLSL, by generating a "main" GLSL shader for the vertex shader, and then an extra function setup_vs_output(), and linking the two together every time a new pixel shader is used. This could in theory be used for SPIR-V, but it requires the use of extra, probably external, code to link SPIR-V shaders together, which I do not particularly anticipate being well-received.
(I'm not sure how Wine deals with this problem in the ARB backend. It seems to take the pixel shader signature into account when generating the vertex shader— cf. init_output_registers()—but it doesn't take it into account when looking up a vertex shader variant? I didn't look too closely at the code, so maybe I'm missing something.)
--
The vkd3d parts of this patch are quite straightforward, and looking at them, I think the design is quite intuitive in isolation. There may be some room for internal refactoring (in particular with an eye to not so much overloading the "register_index" field of struct shader_signature_element) but I'm relatively happy with the way it turned out. In isolation, that is.
The Wine part is worse. I've uploaded branches for vkd3d and Wine that use this API, and correctly handle shaders with some nontrivial reordering:
https://gitlab.winehq.org/zfigura/vkd3d/-/commits/himavant5
https://gitlab.winehq.org/zfigura/wine/-/commits/himavant_cb
The test can be run, as before, like so:
make tests/shader_runner.exe && WINE_D3D_CONFIG=renderer=vulkan wine tests/shader_runner.exe ../vkd3d/tests/hlsl/sm1-interstage-interface.shader_test
The interesting Wine parts are concentrated in a single patch, 5cfb9d930f11e. The patch takes a few shortcuts, partly because I wanted not to block the vkd3d API design questions, but also because while writing it I came up with a couple of problems that I wasn't sure how to fix. There are two main problems I see:
(1) This patch has the user pass the signature from the pixel shader when compiling the vertex shader, and looks up register indices already arbitrarily allocated by the pixel shader. This is problematic when trying to use this signature as a cache key, by virtue of it not being clear (or even defined) which fields are key elements and which aren't. It's also not particularly kind to lookup, on account of not being directly comparable with memcmp(). There are a few options I see:
(a) Provide an internal function to compare key elements. This feels... odd, like a very special-purpose function, but perhaps workable.
(b) Just make the user deal with it, and assert that all fields are key elements.
(c) Use some alternative, perhaps shortened structure as a field of vkd3d_shader_next_stage_info. This has the disadvantage that it is not as simple for a hypothetical user to retrieve from the pixel shader, but we would presumably provide a function to generate one from a shader signature. This would probably also be kinder to cache lookup if it's shorter.
(d) Make caching vkd3d's responsibility, to some degree. This seems daunting, but the more we optimize, the more difficult it may be to design API that allows for nice caching.
(2) Assuming we use signatures, there is a memory management problem that 5cfb9d930f11e spells out. This is probably a matter of "just fix it", but I suppose another option is to take the GLSL or ARB architecture.
--
April is the cruellest month, breeding Lilacs out of the dead land, mixing Memory and desire, stirring Dull roots with spring rain.
-- v6: vkd3d-shader: Introduce a function to build a varying map between sm1 stages. vkd3d-shader/spirv: Make output varyings not consumed by the next stage private variables. vkd3d-shader: Implement remapping shader output registers to match the next shader's semantics. vkd3d-shader: Add a separate field for the target location of a signature element.
From: Zebediah Figura zfigura@codeweavers.com
spirv will need this. --- libs/vkd3d-shader/d3dbc.c | 7 ++++++- tests/vkd3d_shader_api.c | 6 +++--- 2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 9a147e3d..22e463bf 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -524,6 +524,8 @@ static struct signature_element *find_signature_element_by_register_index( return NULL; }
+#define SM1_COLOR_REGISTER_OFFSET 8 + 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) @@ -606,7 +608,7 @@ static bool add_signature_element_from_register(struct vkd3d_shader_sm1_parser * return true; } return add_signature_element(sm1, false, "COLOR", register_index, - VKD3D_SHADER_SV_NONE, register_index, is_dcl, mask); + VKD3D_SHADER_SV_NONE, SM1_COLOR_REGISTER_OFFSET + register_index, is_dcl, mask);
case VKD3DSPR_TEXTURE: /* For vertex shaders, this is ADDR. */ @@ -633,6 +635,9 @@ static bool add_signature_element_from_register(struct vkd3d_shader_sm1_parser * /* fall through */
case VKD3DSPR_ATTROUT: + return add_signature_element(sm1, true, "COLOR", register_index, + VKD3D_SHADER_SV_NONE, SM1_COLOR_REGISTER_OFFSET + register_index, is_dcl, mask); + case VKD3DSPR_COLOROUT: return add_signature_element(sm1, true, "COLOR", register_index, VKD3D_SHADER_SV_NONE, register_index, is_dcl, mask); diff --git a/tests/vkd3d_shader_api.c b/tests/vkd3d_shader_api.c index 8bd728d9..4893f44f 100644 --- a/tests/vkd3d_shader_api.c +++ b/tests/vkd3d_shader_api.c @@ -505,7 +505,7 @@ static void test_scan_signatures(void) { {"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}, + {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 8, 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}, }; @@ -582,7 +582,7 @@ static void test_scan_signatures(void) 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}, + {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 8, 0xf, 0xf}, };
static const struct vkd3d_shader_signature_element ps2_outputs[] = @@ -603,7 +603,7 @@ static void test_scan_signatures(void)
static const struct vkd3d_shader_signature_element ps3_inputs[] = { - {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 0, 0xf, 0xf}, + {"COLOR", 0, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 8, 0xf, 0xf}, {"TEXCOORD", 2, 0, VKD3D_SHADER_SV_NONE, VKD3D_SHADER_COMPONENT_FLOAT, 2, 0xf, 0xf}, };
From: Zebediah Figura zfigura@codeweavers.com
We want to be able to remap input signatures based on the signature index, but signature normalization both reorders the signature, and requires the old register index, so add a new field for this. --- libs/vkd3d-shader/d3dbc.c | 1 + libs/vkd3d-shader/dxbc.c | 1 + libs/vkd3d-shader/spirv.c | 8 ++++---- libs/vkd3d-shader/vkd3d_shader_private.h | 3 +++ 4 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 22e463bf..1277251b 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -557,6 +557,7 @@ static bool add_signature_element(struct vkd3d_shader_sm1_parser *sm1, bool outp element->sysval_semantic = sysval; element->component_type = VKD3D_SHADER_COMPONENT_FLOAT; element->register_index = register_index; + element->target_location = register_index; element->register_count = 1; element->mask = mask; element->used_mask = is_dcl ? 0 : mask; diff --git a/libs/vkd3d-shader/dxbc.c b/libs/vkd3d-shader/dxbc.c index 716b7bdb..cedc3da4 100644 --- a/libs/vkd3d-shader/dxbc.c +++ b/libs/vkd3d-shader/dxbc.c @@ -391,6 +391,7 @@ static int shader_parse_signature(const struct vkd3d_shader_dxbc_section_desc *s read_dword(&ptr, &e[i].sysval_semantic); read_dword(&ptr, &e[i].component_type); read_dword(&ptr, &e[i].register_index); + e[i].target_location = e[i].register_index; e[i].register_count = 1; read_dword(&ptr, &mask); e[i].mask = mask & 0xff; diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index d71f0a69..27d95912 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4602,7 +4602,7 @@ static uint32_t spirv_compiler_emit_input(struct spirv_compiler *compiler, } else { - unsigned int location = signature_element->register_index; + unsigned int location = signature_element->target_location;
input_id = spirv_compiler_emit_array_variable(compiler, &builder->global_stream, storage_class, component_type, input_component_count, array_sizes, 2); @@ -4980,7 +4980,7 @@ static void spirv_compiler_emit_output(struct spirv_compiler *compiler, } else { - unsigned int location = signature_element->register_index; + unsigned int location = signature_element->target_location;
if (is_patch_constant) location += shader_signature_next_location(&compiler->output_signature); @@ -4989,10 +4989,10 @@ static void spirv_compiler_emit_output(struct spirv_compiler *compiler, storage_class, component_type, output_component_count, array_sizes, 2); vkd3d_spirv_add_iface_variable(builder, id);
- if (is_dual_source_blending(compiler) && signature_element->register_index < 2) + if (is_dual_source_blending(compiler) && location < 2) { vkd3d_spirv_build_op_decorate1(builder, id, SpvDecorationLocation, 0); - vkd3d_spirv_build_op_decorate1(builder, id, SpvDecorationIndex, signature_element->register_index); + vkd3d_spirv_build_op_decorate1(builder, id, SpvDecorationIndex, location); } else { diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index d35f49a6..d2a2ffe5 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -815,11 +815,14 @@ struct signature_element unsigned int stream_index; enum vkd3d_shader_sysval_semantic sysval_semantic; enum vkd3d_shader_component_type component_type; + /* Register index in the source shader. */ unsigned int register_index; unsigned int register_count; unsigned int mask; unsigned int used_mask; enum vkd3d_shader_minimum_precision min_precision; + /* Register index / location in the target shader. */ + unsigned int target_location; };
struct shader_signature
From: Zebediah Figura zfigura@codeweavers.com
--- include/vkd3d_shader.h | 78 +++++++++++++++++++++++- libs/vkd3d-shader/ir.c | 77 ++++++++++++++++++++++- libs/vkd3d-shader/spirv.c | 2 +- libs/vkd3d-shader/vkd3d_shader_private.h | 3 +- 4 files changed, 156 insertions(+), 4 deletions(-)
diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h index d6653d18..9b027e9f 100644 --- a/include/vkd3d_shader.h +++ b/include/vkd3d_shader.h @@ -90,6 +90,11 @@ enum vkd3d_shader_structure_type * \since 1.9 */ VKD3D_SHADER_STRUCTURE_TYPE_SCAN_SIGNATURE_INFO, + /** + * The structure is a vkd3d_shader_next_stage_info structure. + * \since 1.9 + */ + VKD3D_SHADER_STRUCTURE_TYPE_NEXT_STAGE_INFO,
VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_STRUCTURE_TYPE), }; @@ -1676,6 +1681,76 @@ struct vkd3d_shader_scan_signature_info struct vkd3d_shader_signature patch_constant; };
+/** + * Describes the mapping of a output varying register in a shader stage, + * to an input varying register in the following shader stage. + * + * This structure is used in struct vkd3d_shader_next_stage_info. + */ +struct vkd3d_shader_varying_map +{ + /** + * The signature index (in the output signature) of the output varying. + * If greater than or equal to the number of elements in the output + * signature, signifies that the varying is consumed by the next stage but + * not written by this one. + */ + unsigned int output_signature_index; + /** The register index of the input varying to map this register to. */ + unsigned int input_register_index; + /** The mask consumed by the destination register. */ + unsigned int input_mask; +}; + +/** + * A chained structure which describes the next shader in the pipeline. + * + * This structure is optional, and should only be provided if there is in fact + * another shader in the pipeline. + * However, depending on the input and output formats, this structure may be + * necessary in order to generate shaders which correctly match each other. + * If the structure or its individual fields are not provided, vkd3d-shader + * will generate shaders which may be correct in isolation, but are not + * guaranteed to correctly match each other. + * + * For example, when compiling legacy Direct3D bytecode to SPIR-V, vkd3d-shader + * needs to assign a mapping of Direct3D usages to SPIR-V varying locations, + * and because legacy Direct3D bytecode need not specify locations in the same + * order between stages (unlike e.g. TPF) vkd3d-shader needs to know that + * mapping when compiling linked stages. + * + * This structure is passed to vkd3d_shader_compile() and extends + * vkd3d_shader_compile_info. + * + * This structure contains only input parameters. + * + * \since 1.9 + */ +struct vkd3d_shader_next_stage_info +{ + /** Must be set to VKD3D_SHADER_STRUCTURE_TYPE_NEXT_STAGE_INFO. */ + enum vkd3d_shader_structure_type type; + /** Optional pointer to a structure containing further parameters. */ + const void *next; + + /** + * A mapping of output varyings in this shader stage to input varyings + * in the next shader stage. + * + * This mapping should include exactly one element for each varying + * consumed by the next shader stage. + * If this shader stage outputs a varying that is not consumed by the next + * shader stage, that varying should be absent from this array. + * + * This field must be provided when compiling from legacy Direct3D + * bytecode, if there is another shader stage (i.e. it must be provided for + * vertex shaders). + */ + const struct vkd3d_shader_varying_map *varying_map; + /** The number of registers provided in \ref varying_map. */ + unsigned int varying_count; +}; + #ifdef LIBVKD3D_SHADER_SOURCE # define VKD3D_SHADER_API VKD3D_EXPORT #else @@ -1748,13 +1823,14 @@ VKD3D_SHADER_API const enum vkd3d_shader_target_type *vkd3d_shader_get_supported * * Depending on the source and target types, this function may support the * following chained structures: + * - vkd3d_shader_hlsl_source_info * - vkd3d_shader_interface_info + * - vkd3d_shader_next_stage_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 - * - vkd3d_shader_hlsl_source_info * * \param compile_info A chained structure containing compilation parameters. * diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index d74f81af..2af01403 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -85,6 +85,76 @@ static void shader_instruction_eliminate_phase_instance_id(struct vkd3d_shader_i shader_register_eliminate_phase_addressing((struct vkd3d_shader_register *)&ins->dst[i].reg, instance_id); }
+static const struct vkd3d_shader_varying_map *find_varying_map( + const struct vkd3d_shader_next_stage_info *next_stage, unsigned int signature_idx) +{ + unsigned int i; + + for (i = 0; i < next_stage->varying_count; ++i) + { + if (next_stage->varying_map[i].output_signature_index == signature_idx) + return &next_stage->varying_map[i]; + } + + return NULL; +} + +static enum vkd3d_result remap_output_signature(struct shader_signature *signature, + const struct vkd3d_shader_compile_info *compile_info) +{ + const struct vkd3d_shader_next_stage_info *next_stage; + unsigned int i; + + if (!(next_stage = vkd3d_find_struct(compile_info->next, NEXT_STAGE_INFO))) + { + /* This is an error for sm1 shaders, but acceptable for sm4. */ + if (compile_info->source_type == VKD3D_SHADER_SOURCE_D3D_BYTECODE) + { + ERR("Next stage information was not provided.\n"); + return VKD3D_ERROR_INVALID_ARGUMENT; + } + + return VKD3D_OK; + } + + for (i = 0; i < signature->element_count; ++i) + { + const struct vkd3d_shader_varying_map *map = find_varying_map(next_stage, i); + struct signature_element *e = &signature->elements[i]; + + if (map) + { + unsigned int input_mask = map->input_mask; + + e->target_location = map->input_register_index; + + /* It is illegal in Vulkan if the next shader uses the same varying + * location with a different mask. */ + if (input_mask && input_mask != e->mask) + { + FIXME("Output mask %#x does not match input mask %#x.\n", e->mask, input_mask); + return VKD3D_ERROR_NOT_IMPLEMENTED; + } + } + else + { + FIXME("This stage outputs varyings not consumed by the next stage.\n"); + return VKD3D_ERROR_NOT_IMPLEMENTED; + } + } + + for (i = 0; i < next_stage->varying_count; ++i) + { + if (next_stage->varying_map[i].output_signature_index >= signature->element_count) + { + FIXME("The next stage consumes varyings not written by this stage.\n"); + return VKD3D_ERROR_NOT_IMPLEMENTED; + } + } + + return VKD3D_OK; +} + struct hull_flattener { struct vkd3d_shader_instruction_array instructions; @@ -1194,7 +1264,8 @@ static enum vkd3d_result instruction_array_normalise_flat_constants(struct vkd3d return VKD3D_OK; }
-enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser) +enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, + const struct vkd3d_shader_compile_info *compile_info) { struct vkd3d_shader_instruction_array *instructions = &parser->instructions; enum vkd3d_result result = VKD3D_OK; @@ -1202,6 +1273,10 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser) if (parser->shader_desc.is_dxil) return result;
+ if (parser->shader_version.type != VKD3D_SHADER_TYPE_PIXEL + && (result = remap_output_signature(&parser->shader_desc.output_signature, compile_info)) < 0) + return result; + if (parser->shader_version.type == VKD3D_SHADER_TYPE_HULL && (result = instruction_array_flatten_hull_shader_phases(instructions)) >= 0) { diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 27d95912..28825256 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -9542,7 +9542,7 @@ static int spirv_compiler_generate_spirv(struct spirv_compiler *compiler, compiler->location.column = 0; compiler->location.line = 1;
- if ((result = vkd3d_shader_normalise(parser)) < 0) + if ((result = vkd3d_shader_normalise(parser, compile_info)) < 0) return result;
instructions = parser->instructions; diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index d2a2ffe5..ed344332 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1409,6 +1409,7 @@ void dxbc_writer_add_section(struct dxbc_writer *dxbc, uint32_t tag, const void void dxbc_writer_init(struct dxbc_writer *dxbc); int dxbc_writer_write(struct dxbc_writer *dxbc, struct vkd3d_shader_code *code);
-enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser); +enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, + const struct vkd3d_shader_compile_info *compile_info);
#endif /* __VKD3D_SHADER_PRIVATE_H */
From: Zebediah Figura zfigura@codeweavers.com
--- libs/vkd3d-shader/ir.c | 3 +-- libs/vkd3d-shader/spirv.c | 6 ++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 5 ++++- 3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 2af01403..baec2631 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -138,8 +138,7 @@ static enum vkd3d_result remap_output_signature(struct shader_signature *signatu } else { - FIXME("This stage outputs varyings not consumed by the next stage.\n"); - return VKD3D_ERROR_NOT_IMPLEMENTED; + e->target_location = SIGNATURE_TARGET_LOCATION_UNUSED; } }
diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 28825256..2725ed80 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4978,6 +4978,12 @@ static void spirv_compiler_emit_output(struct spirv_compiler *compiler,
spirv_compiler_emit_register_execution_mode(compiler, &dst->reg); } + else if (signature_element->target_location == SIGNATURE_TARGET_LOCATION_UNUSED) + { + storage_class = SpvStorageClassPrivate; + id = spirv_compiler_emit_array_variable(compiler, &builder->global_stream, + storage_class, component_type, output_component_count, array_sizes, 2); + } else { unsigned int location = signature_element->target_location; diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index ed344332..9ebb2a92 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -807,6 +807,8 @@ enum vkd3d_shader_input_sysval_semantic VKD3D_SIV_LINE_DENSITY_TESS_FACTOR = 22, };
+#define SIGNATURE_TARGET_LOCATION_UNUSED (~0u) + struct signature_element { unsigned int sort_index; @@ -821,7 +823,8 @@ struct signature_element unsigned int mask; unsigned int used_mask; enum vkd3d_shader_minimum_precision min_precision; - /* Register index / location in the target shader. */ + /* Register index / location in the target shader. + * If SIGNATURE_TARGET_LOCATION_UNUSED, this element should not be written. */ unsigned int target_location; };
From: Zebediah Figura zfigura@codeweavers.com
--- include/vkd3d_shader.h | 31 ++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_main.c | 38 +++++++++++++++++++++++++++ 2 files changed, 69 insertions(+)
diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h index 9b027e9f..21eb20d5 100644 --- a/include/vkd3d_shader.h +++ b/include/vkd3d_shader.h @@ -1742,6 +1742,8 @@ struct vkd3d_shader_next_stage_info * If this shader stage outputs a varying that is not consumed by the next * shader stage, that varying should be absent from this array. * + * This mapping may be constructed by vkd3d_shader_build_varying_map(). + * * This field must be provided when compiling from legacy Direct3D * bytecode, if there is another shader stage (i.e. it must be provided for * vertex shaders). @@ -2264,6 +2266,35 @@ VKD3D_SHADER_API int vkd3d_shader_serialize_dxbc(size_t section_count, */ VKD3D_SHADER_API void vkd3d_shader_free_scan_signature_info(struct vkd3d_shader_scan_signature_info *info);
+/** + * Build a mapping of output varyings in a shader stage to input varyings in + * the following shader stage. + * + * This mapping should be used in struct vkd3d_shader_next_stage_info to + * compile the first shader. + * + * \param output_signature The output signature of the first shader. + * + * \param input_signature The input signature of the second shader. + * + * \param count On output, contains the number of entries written into + * \ref varyings. + * + * \param varyings Pointer to an output array of varyings. + * This must point to space for N varyings, where N is the number of elements + * in the input signature. + * + * \remark Valid legacy Direct3D pixel shaders have at most 12 varying inputs: + * 10 inter-stage varyings, face, and position. + * Therefore, in practice, it is safe to call this function with a + * pre-allocated array with a fixed size of 12. + * + * \since 1.9 + */ +VKD3D_SHADER_API void vkd3d_shader_build_varying_map(const struct vkd3d_shader_signature *output_signature, + const struct vkd3d_shader_signature *input_signature, + unsigned int *count, struct vkd3d_shader_varying_map *varyings); + #endif /* VKD3D_SHADER_NO_PROTOTYPES */
/** Type of vkd3d_shader_get_version(). */ diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index e8259181..e6ffb84d 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -1891,3 +1891,41 @@ void shader_instruction_array_destroy(struct vkd3d_shader_instruction_array *ins vkd3d_free(instructions->icbs[i]); vkd3d_free(instructions->icbs); } + +void vkd3d_shader_build_varying_map(const struct vkd3d_shader_signature *output_signature, + const struct vkd3d_shader_signature *input_signature, + unsigned int *ret_count, struct vkd3d_shader_varying_map *varyings) +{ + unsigned int count = 0; + unsigned int i; + + TRACE("output_signature %p, input_signature %p, ret_count %p, varyings %p.\n", + output_signature, input_signature, ret_count, varyings); + + for (i = 0; i < input_signature->element_count; ++i) + { + const struct vkd3d_shader_signature_element *input_element, *output_element; + + input_element = &input_signature->elements[i]; + + if (input_element->sysval_semantic != VKD3D_SHADER_SV_NONE) + continue; + + varyings[count].input_register_index = input_element->register_index; + varyings[count].input_mask = input_element->mask; + + if ((output_element = vkd3d_shader_find_signature_element(output_signature, + input_element->semantic_name, input_element->semantic_index, 0))) + { + varyings[count].output_signature_index = output_element - output_signature->elements; + } + else + { + varyings[count].output_signature_index = output_signature->element_count; + } + + ++count; + } + + *ret_count = count; +}
Just some nits:
+ /** The mask consumed by the destination register. */ + unsigned int input_mask;
This matches struct vkd3d_shader_signature_element, so in that sense it fine. I could also see an argument for making "input_mask" a uint32_t though.
+ /** + * A mapping of output varyings in this shader stage to input varyings + * in the next shader stage. + * + * This mapping should include exactly one element for each varying + * consumed by the next shader stage. + * If this shader stage outputs a varying that is not consumed by the next + * shader stage, that varying should be absent from this array. + * + * This field must be provided when compiling from legacy Direct3D + * bytecode, if there is another shader stage (i.e. it must be provided for + * vertex shaders). + */ + const struct vkd3d_shader_varying_map *varying_map; + /** The number of registers provided in \ref varying_map. */ + unsigned int varying_count;
I think I raised this before, but I'm not sure we actually resolved it: Shader model 1 and 2 shaders have a fixed mapping between output registers and their semantics; e.g. oT3 in the vertex shader will always be TEXCOORD3, and so will t3 in the pixel shader. There may certainly be reasons for wanting to remap outputs anyway, but in principle on Vulkan/SPIR-V capable hardware, compiling e.g. a shader model 2 vertex shader should produce something that can be used by the corresponding shader model 2 pixel shader, without any remapping being needed. I.e., I think it's specifically shader model 3 vertex shaders where we need this.
It's perhaps worth considering usage by something like vkd3d-compiler as well. We'll want to provide a way for vkd3d-compiler to specify a mapping, and creating an identity mapping when none is provided by the user shouldn't be too hard, but not having to specify one would be slightly easier.
+ /* This is an error for sm1 shaders, but acceptable for sm4. */ + if (compile_info->source_type == VKD3D_SHADER_SOURCE_D3D_BYTECODE) + { + ERR("Next stage information was not provided.\n"); + return VKD3D_ERROR_INVALID_ARGUMENT; + }
See also above, but we should never use ERR for something caused by data provided (or not) by the user of the API. If we're going to error out here, vkd3d_shader_error() would be the way to do it. That essentially applies to the FIXMEs in the same function as well.
Just some nits:
+ /** The mask consumed by the destination register. */ + unsigned int input_mask;This matches struct vkd3d_shader_signature_element, so in that sense it fine. I could also see an argument for making "input_mask" a uint32_t though.
I'm inclined to stick to consistent types.
(Though we could probably also change both to uint32_t without actually breaking API?)
+ /** + * A mapping of output varyings in this shader stage to input varyings + * in the next shader stage. + * + * This mapping should include exactly one element for each varying + * consumed by the next shader stage. + * If this shader stage outputs a varying that is not consumed by the next + * shader stage, that varying should be absent from this array. + * + * This field must be provided when compiling from legacy Direct3D + * bytecode, if there is another shader stage (i.e. it must be provided for + * vertex shaders). + */ + const struct vkd3d_shader_varying_map *varying_map; + /** The number of registers provided in \ref varying_map. */ + unsigned int varying_count;I think I raised this before, but I'm not sure we actually resolved it: Shader model 1 and 2 shaders have a fixed mapping between output registers and their semantics; e.g. oT3 in the vertex shader will always be TEXCOORD3, and so will t3 in the pixel shader. There may certainly be reasons for wanting to remap outputs anyway, but in principle on Vulkan/SPIR-V capable hardware, compiling e.g. a shader model 2 vertex shader should produce something that can be used by the corresponding shader model 2 pixel shader, without any remapping being needed. I.e., I think it's specifically shader model 3 vertex shaders where we need this.
In terms of mapping registers, yes, although this is also used to communicate some (probably exceptional cases), namely, semantics not produced by the VS, and mismatched masks.
I suppose we could say that it's a hard requirement if the shader is 3.0 or (eventually) if the target is GLSL or ARB. Of course such a condition can get arbitrarily complex.
It feels awkward to me to be that selective about it, because it *might* be necessary in all kinds of cases, and I doubt the caller is likely to *know* if it's necessary. Which is why I originally just wrote "should", and then later changed it to "must" with a blanket prohibition on not providing that information.
It's perhaps worth considering usage by something like vkd3d-compiler as well. We'll want to provide a way for vkd3d-compiler to specify a mapping, and creating an identity mapping when none is provided by the user shouldn't be too hard, but not having to specify one would be slightly easier.
I did very briefly consider vkd3d-compiler usage, and I think the most obvious API that comes to mind is an option to provide the next stage binary, which vkd3d-compiler will use to reflect and then call vkd3d_shader_build_varying_map(). I suppose manually specifying a more sophisticated mapping is possible, although I don't know whether it's reasonably in the scope of a command line tool.
+ /* This is an error for sm1 shaders, but acceptable for sm4. */ + if (compile_info->source_type == VKD3D_SHADER_SOURCE_D3D_BYTECODE) + { + ERR("Next stage information was not provided.\n"); + return VKD3D_ERROR_INVALID_ARGUMENT; + }See also above, but we should never use ERR for something caused by data provided (or not) by the user of the API. If we're going to error out here, vkd3d_shader_error() would be the way to do it. That essentially applies to the FIXMEs in the same function as well.
Sure, will change.
-
Henri Verbeet (＠hverbeet) -
Zebediah Figura
-
Zebediah Figura (＠zfigura)