[PATCH 0/2] MR280: vkd3d-shader: sm1 semantic matching.
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/280
From: Zebediah Figura <zfigura(a)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}, }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/280
From: Zebediah Figura <zfigura(a)codeweavers.com> --- configure.ac | 1 + include/private/vkd3d_common.h | 13 +++++++++ include/vkd3d_shader.h | 50 +++++++++++++++++++++++++++++++++- libs/vkd3d-shader/spirv.c | 50 ++++++++++++++++++++++++++++++++++ 4 files changed, 113 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index b58c2f4d..4a5e1f05 100644 --- a/configure.ac +++ b/configure.ac @@ -138,6 +138,7 @@ AS_IF([test "x$with_xcb" != "xno"], dnl Check for functions VKD3D_CHECK_FUNC([HAVE_BUILTIN_CLZ], [__builtin_clz], [__builtin_clz(0)]) +VKD3D_CHECK_FUNC([HAVE_BUILTIN_FFS], [__builtin_ffs], [__builtin_ffs(1)]) VKD3D_CHECK_FUNC([HAVE_BUILTIN_POPCOUNT], [__builtin_popcount], [__builtin_popcount(0)]) VKD3D_CHECK_FUNC([HAVE_BUILTIN_ADD_OVERFLOW], [__builtin_add_overflow], [__builtin_add_overflow(0, 0, (int *)0)]) VKD3D_CHECK_FUNC([HAVE_SYNC_ADD_AND_FETCH], [__sync_add_and_fetch], [__sync_add_and_fetch((int *)0, 0)]) diff --git a/include/private/vkd3d_common.h b/include/private/vkd3d_common.h index f3242272..7098ff54 100644 --- a/include/private/vkd3d_common.h +++ b/include/private/vkd3d_common.h @@ -148,6 +148,19 @@ static inline unsigned int vkd3d_log2i(unsigned int x) #endif } +static inline int vkd3d_bit_scan(unsigned int *x) +{ + int bit_offset; +#ifdef HAVE_BUILTIN_FFS + bit_offset = __builtin_ffs(*x) - 1; +#else + for (bit_offset = 0; bit_offset < 32; bit_offset++) + if (*x & (1u << bit_offset)) break; +#endif + *x ^= 1u << bit_offset; + return bit_offset; +} + static inline void *vkd3d_memmem( const void *haystack, size_t haystack_len, const void *needle, size_t needle_len) { const char *str = haystack; diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h index 8133d240..cd569ce9 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), }; @@ -1659,6 +1664,48 @@ struct vkd3d_shader_scan_signature_info struct vkd3d_shader_signature patch_constant; }; +/** + * 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 should be provided when compiling from + * VKD3D_SHADER_SOURCE_D3D_BYTECODE. + * + * 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; + + /** + * The input signature of the next shader. + * This should be generated by vkd3d-shader, using + * struct vkd3d_shader_scan_signature_info. + */ + const struct vkd3d_shader_signature *input_signature; +}; + #ifdef LIBVKD3D_SHADER_SOURCE # define VKD3D_SHADER_API VKD3D_EXPORT #else @@ -1731,13 +1778,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/spirv.c b/libs/vkd3d-shader/spirv.c index dcf957ea..5158dec5 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -9455,6 +9455,53 @@ static void spirv_compiler_emit_sm1_constant_buffer(struct spirv_compiler *compi spirv_compiler_emit_constant_buffer(compiler, count, &range, ®); } +/* Find the output semantic register index matching the next shader, or + * find an unused register index if the next shader doesn't consume this + * semantic. */ +static unsigned int get_output_semantic_register_index(const struct vkd3d_shader_signature *ps_signature, + const char *name, unsigned int index, uint32_t *output_register_mask) +{ + struct vkd3d_shader_signature_element *element; + + if ((element = vkd3d_shader_find_signature_element(ps_signature, name, index, 0))) + return element->register_index; + + if (*output_register_mask) + return vkd3d_bit_scan(output_register_mask); + + ERR("No unused register index available.\n"); + return ~0u; +} + +static void remap_output_signature(struct shader_signature *signature, + const struct vkd3d_shader_compile_info *compile_info) +{ + const struct vkd3d_shader_next_stage_info *next_stage_info; + const struct vkd3d_shader_signature *ps_signature; + uint32_t output_register_mask = ~0u; + unsigned int i; + + if (!(next_stage_info = vkd3d_find_struct(compile_info->next, NEXT_STAGE_INFO)) + || !(ps_signature = next_stage_info->input_signature)) + return; + + for (i = 0; i < ps_signature->element_count; ++i) + { + unsigned int register_index = ps_signature->elements[i].register_index; + + if (register_index < 32) + bitmap_clear(&output_register_mask, register_index); + } + + for (i = 0; i < signature->element_count; ++i) + { + struct signature_element *e = &signature->elements[i]; + + e->register_index = get_output_semantic_register_index(ps_signature, + e->semantic_name, e->semantic_index, &output_register_mask); + } +} + static int spirv_compiler_generate_spirv(struct spirv_compiler *compiler, const struct vkd3d_shader_compile_info *compile_info, struct vkd3d_shader_parser *parser, struct vkd3d_shader_code *spirv) @@ -9486,6 +9533,9 @@ static int spirv_compiler_generate_spirv(struct spirv_compiler *compiler, instructions = parser->instructions; memset(&parser->instructions, 0, sizeof(parser->instructions)); + if (parser->shader_version.major < 4 && parser->shader_version.type == VKD3D_SHADER_TYPE_VERTEX) + remap_output_signature(&shader_desc->output_signature, compile_info); + compiler->input_signature = shader_desc->input_signature; compiler->output_signature = shader_desc->output_signature; compiler->patch_constant_signature = shader_desc->patch_constant_signature; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/280
(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. ```
Yeah, the interstage interface is probably one of the harder parts to get right. So it seems like what we're ultimately going to do with NEXT_STAGE_INFO is to construct an output map for the vertex shader. One (perhaps obvious) alternative then would be to pass that output map to vkd3d-shader directly, instead of NEXT_STAGE_INFO. The main advantage would be that such an output map could be stored in a fairly compact way if needed, and could easily be compared with memcmp(). There may also be variants we could do based on that. E.g., keep passing NEXT_STAGE_INFO to vkd3d_shader_compile(), but introduce a function that returns an output map consistent with what vkd3d_shader_compile() would use for a given signature pair. Or we could return the output map that would be used from vkd3d_shader_scan().
(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.
We could, and it might not even be that hard, but it would of course introduce some extra allocations and copying that would be superfluous for more typical usage. In particular, the expectation is that we have essentially two kinds of uses of the API/structure: - Users that keep neither the signature nor the shader around. E.g. because the user of the API is just printing some information about the shader, or e.g. because the user of the API is using its own structures to store whatever information it actually cares about. - Users that keep both the signature and the shader around. E.g. because the API user isn't particularly concerned about just keeping the shader around, or because it needs to keep the shader around anyway because it will be needed again later. I.e., the expectation is that it should be fairly uncommon to have a need to hold on to a struct vkd3d_shader_signature, but not the corresponding shader. Note that "just fix it" doesn't make the issue in the commit in question that much easier to fix either; we'd still need to keep track of when it's safe to release the signature. We could wrap the signature in some structure with a refcount in wined3d, but in that case we could also just keep a reference to the shader the signature belongs to, and that might not end up being much worse in practice. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/280#note_39539
So it seems like what we're ultimately going to do with NEXT_STAGE_INFO is to construct an output map for the vertex shader. One (perhaps obvious) alternative then would be to pass that output map to vkd3d-shader directly, instead of NEXT_STAGE_INFO. The main advantage would be that such an output map could be stored in a fairly compact way if needed, and could easily be compared with memcmp().
There may also be variants we could do based on that. E.g., keep passing NEXT_STAGE_INFO to vkd3d_shader_compile(), but introduce a function that returns an output map consistent with what vkd3d_shader_compile() would use for a given signature pair. Or we could return the output map that would be used from vkd3d_shader_scan().
Yeah, that was broadly what I was trying to propose with (c). Not sure if I should take this as an endorsement or preference of that approach, but I at least can't say I like anything else better, so I may just try to start writing an implementation thereof. Side note: maybe this should be a vkd3d_shader_parameter instead? I think it'd probably be good to lean in that direction, rather than adding more chained structures, for new compilation options like this. Mostly because it's easier to design and, I think, easier to use. It'll require rerolling vkd3d_shader_parameter which unfortunately can only have uint32_t in it, but we needed that anyway. As a side note—when I wrote vkd3d_shader_next_stage_info I had in mind that it could be extended with other aspects of the next shader. The only one of these that I can currently actually think of is the shader type, which I think GLSL cares about sometimes.
We could, and it might not even be that hard, but it would of course introduce some extra allocations and copying that would be superfluous for more typical usage. In particular, the expectation is that we have essentially two kinds of uses of the API/structure:
* Users that keep neither the signature nor the shader around. E.g. because the user of the API is just printing some information about the shader, or e.g. because the user of the API is using its own structures to store whatever information it actually cares about. * Users that keep both the signature and the shader around. E.g. because the API user isn't particularly concerned about just keeping the shader around, or because it needs to keep the shader around anyway because it will be needed again later.
I.e., the expectation is that it should be fairly uncommon to have a need to hold on to a struct vkd3d_shader_signature, but not the corresponding shader.
It's a fair point, although I am still sort of worried that an API user isn't going to see the design in that terms, especially not a priori (I mean, I didn't), and we'd risk needlessly frustrating with what looks like a bug. Or, perhaps more saliently, I could see someone assuming (despite the documentation) or forgetting about that implicit dependency, and causing subtle UAF problems as a result. Or I could see an argument for someone wanting to, say, compile a shader into SPIRV, keep the compiled shader and the signature around, and throw away the original. Which is not me trying to belabour the argument in favor of "fixing" the "bug"—I'm not even fully convinced that it needs fixing either—but it does make me nervous.
Note that "just fix it" doesn't make the issue in the commit in question that much easier to fix either; we'd still need to keep track of when it's safe to release the signature. We could wrap the signature in some structure with a refcount in wined3d, but in that case we could also just keep a reference to the shader the signature belongs to, and that might not end up being much worse in practice.
Also true. I can't claim to have fully thought this part out, but I think I was figuring that we'd just keep the list of variants around forever, even if one or more of the corresponding vertex shaders is destroyed. [Especially because... how likely is it that the VS's even will be?] There is a performance question here—do we care about avoiding shader recompilation when multiple different PS's with identical signatures are used? Wine seems to care, at least enough to avoid recompiling the VS into GLSL. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/280#note_39563
So it seems like what we're ultimately going to do with NEXT_STAGE_INFO is to construct an output map for the vertex shader. One (perhaps obvious) alternative then would be to pass that output map to vkd3d-shader directly, instead of NEXT_STAGE_INFO. The main advantage would be that such an output map could be stored in a fairly compact way if needed, and could easily be compared with memcmp().
There may also be variants we could do based on that. E.g., keep passing NEXT_STAGE_INFO to vkd3d_shader_compile(), but introduce a function that returns an output map consistent with what vkd3d_shader_compile() would use for a given signature pair. Or we could return the output map that would be used from vkd3d_shader_scan().
Yeah, that was broadly what I was trying to propose with (c).
Not sure if I should take this as an endorsement or preference of that approach, but I at least can't say I like anything else better, so I may just try to start writing an implementation thereof.
I'm mostly just proposing ideas at this stage. Something like this seems the most workable to me, but I haven't entirely thought it through, and it's entirely possible that better alternatives exist. To the extent that I have a preference, I'm leaning towards the option of keeping NEXT_STAGE_INFO and then returning the resulting output map from vkd3d_shader_scan(), but it's only a weak preference at this point.
Side note: maybe this should be a vkd3d_shader_parameter instead? I think it'd probably be good to lean in that direction, rather than adding more chained structures, for new compilation options like this. Mostly because it's easier to design and, I think, easier to use. It'll require rerolling vkd3d_shader_parameter which unfortunately can only have uint32_t in it, but we needed that anyway.
What would that look like? Shader parameters were roughly intended as the vkd3d-shader equivalent of SPIR-V specialisation constants; it would probably make sense to use them for e.g. spirv_compiler_emit_sample_position(), but it's less clear to me whether the interstage interface would be a great fit.
As a side note—when I wrote vkd3d_shader_next_stage_info I had in mind that it could be extended with other aspects of the next shader. The only one of these that I can currently actually think of is the shader type, which I think GLSL cares about sometimes.
Yeah, I'd suggest adding the shader type if we're going with NEXT_STAGE_INFO; I haven't commented much on the specifics so far, mostly just in order to focus on the broader questions first. I seem to recall that there may also be some cases where we'd need to initialise vertex shader outputs that aren't otherwise written by the vertex shader, but I'd need to check that.
Or, perhaps more saliently, I could see someone assuming (despite the documentation) or forgetting about that implicit dependency, and causing subtle UAF problems as a result.
Or I could see an argument for someone wanting to, say, compile a shader into SPIRV, keep the compiled shader and the signature around, and throw away the original.
Which is not me trying to belabour the argument in favor of "fixing" the "bug"—I'm not even fully convinced that it needs fixing either—but it does make me nervous.
Yeah, I think those were concerns at the time as well. Note that if we do decide to change this there may be broader implications as well; we do this kind of thing in a number of other places as well.
There is a performance question here—do we care about avoiding shader recompilation when multiple different PS's with identical signatures are used? Wine seems to care, at least enough to avoid recompiling the VS into GLSL.
Generally, yes. I think the driver part of shader compilation is probably more expensive than anything we do, but we can at least attempt to not make it worse or to mitigate it somewhat. And while much of the world seems to have thrown up their hands and said "well, shader caches" at the problem of shader/pipeline compilation stutter, we do in theory have VK_EXT_shader_object now. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/280#note_39627
Side note: maybe this should be a vkd3d_shader_parameter instead? I think it'd probably be good to lean in that direction, rather than adding more chained structures, for new compilation options like this. Mostly because it's easier to design and, I think, easier to use. It'll require rerolling vkd3d_shader_parameter which unfortunately can only have uint32_t in it, but we needed that anyway.
What would that look like? Shader parameters were roughly intended as the vkd3d-shader equivalent of SPIR-V specialisation constants; it would probably make sense to use them for e.g. spirv_compiler_emit_sample_position(), but it's less clear to me whether the interstage interface would be a great fit.
That's the form of shader parameters, but I don't think it makes any statements about their function. I don't know exactly what the intent for the function was, but honestly I'm not sure I see a reason to limit it. It can just mean "any input parameter that reflects something about the source environment that the target type requires to be encoded in the shader". That may mean that vkd3d_shader_parameter could in theory cover almost everything, but I'm not sure I see that as a bad thing. I mean, the thing about chained structures is that they're not exactly the easiest thing to work with, mostly when it comes to optionally adding them, or building helpers to construct the parameters. Arrays like vkd3d_shader_parameter are nicer. It also seems like adding one parameter at a time via vkd3d_shader_parameter requires less boilerplate than adding one at a time via chained structs, and adding several at a time is less arbitrary. FWIW, I was planning to make things like alpha test or flat shading into parameters as well... just to give an idea of what else we may have to reconsider if gonig a different direction. As for what it would look like, well, for a vague diff that leaves out important pieces, something like ``` diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h index cd569ce9..ba8a9a78 100644 --- a/include/vkd3d_shader.h +++ b/include/vkd3d_shader.h @@ -292,6 +292,8 @@ enum vkd3d_shader_parameter_type VKD3D_SHADER_PARAMETER_TYPE_UNKNOWN, VKD3D_SHADER_PARAMETER_TYPE_IMMEDIATE_CONSTANT, VKD3D_SHADER_PARAMETER_TYPE_SPECIALIZATION_CONSTANT, + VKD3D_SHADER_PARAMETER_TYPE_VARYING_MAPPING, + VKD3D_SHADER_PARAMETER_TYPE_ALPHA_TEST_FUNC, VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_PARAMETER_TYPE), }; @@ -308,6 +310,8 @@ enum vkd3d_shader_parameter_name { VKD3D_SHADER_PARAMETER_NAME_UNKNOWN, VKD3D_SHADER_PARAMETER_NAME_RASTERIZER_SAMPLE_COUNT, + VKD3D_SHADER_PARAMETER_NAME_OUTPUT_VARYING_REMAP, + VKD3D_SHADER_PARAMETER_NAME_ALPHA_TEST_FUNC, VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_PARAMETER_NAME), }; @@ -334,6 +338,8 @@ struct vkd3d_shader_parameter { struct vkd3d_shader_parameter_immediate_constant immediate_constant; struct vkd3d_shader_parameter_specialization_constant specialization_constant; + struct vkd3d_shader_varying_mapping varying_mapping; + enum vkd3d_shader_alpha_test_func alpha_test_func; } u; }; ``` and the data type could be UNKNOWN or we could, I suppose, add dedicated data types. In this case not every parameter name can support every parameter type, and I can see why that would be distasteful. In that case I guess alpha ref could belong as a vkd3d_shader_parameter, and everything else would need to be relegated to chained structures. I don't feel that distaste personally, but I won't argue about it. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/280#note_39657
That's the form of shader parameters, but I don't think it makes any statements about their function. I don't know exactly what the intent for the function was, but honestly I'm not sure I see a reason to limit it. It can just mean "any input parameter that reflects something about the source environment that the target type requires to be encoded in the shader".
Sure, but at that point you're essentially just arguing for passing the structures in question as an array instead of as a linked list. And struct vkd3d_shader_compile_option could be extended in a similar way to pass arbitrary data, of course.
That may mean that vkd3d_shader_parameter could in theory cover almost everything, but I'm not sure I see that as a bad thing. I mean, the thing about chained structures is that they're not exactly the easiest thing to work with, mostly when it comes to optionally adding them, or building helpers to construct the parameters. Arrays like vkd3d_shader_parameter are nicer.
I don't necessarily agree. Sure, constructing these chains has its issues, but they're issues that should be familiar to someone used to Vulkan, and I wouldn't say these issues are worse than you'd have with vkd3d_shader_parameter or vkd3d_shader_compile_option.
```diff @@ -334,6 +338,8 @@ struct vkd3d_shader_parameter { struct vkd3d_shader_parameter_immediate_constant immediate_constant; struct vkd3d_shader_parameter_specialization_constant specialization_constant; + struct vkd3d_shader_varying_mapping varying_mapping; + enum vkd3d_shader_alpha_test_func alpha_test_func; } u; }; ```
That doesn't scale, generally. Adding things to the union potentially changes the size of the vkd3d_shader_parameter structure, and that's a problem if we want to both pass an array of these and maintain a stable ABI.
In this case not every parameter name can support every parameter type, and I can see why that would be distasteful.
Well, it would be a break from how these are currently intended to be used, at least. These were intended mostly to provide an option other than "recompile the shader" and "use a constant buffer" for almost constant shader variables. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/280#note_39664
I don't necessarily agree. Sure, constructing these chains has its issues, but they're issues that should be familiar to someone used to Vulkan, and I wouldn't say these issues are worse than you'd have with vkd3d_shader_parameter or vkd3d_shader_compile_option.
It's a fair point, and one I should not lose sight of. Anyway, I'll try to write a new version now, I suppose I don't see any remaining unsolved questions for the moment. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/280#note_39707
participants (3)
-
Henri Verbeet (＠hverbeet) -
Zebediah Figura
-
Zebediah Figura (＠zfigura)