Skip to content
Snippets Groups Projects

vkd3d-shader: sm1 semantic matching.

Merged Elizabeth Figura requested to merge zfigura/vkd3d:pr0 into master

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.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • (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.

  • Author Developer

    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.

  • 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.

  • Author Developer

    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.

  • 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.

    @@ -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.

  • Author Developer

    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.

  • Elizabeth Figura added 2 commits

    added 2 commits

    • 3d5171d8 - vkd3d-shader: Implement remapping shader output registers to match the next shader's semantics.
    • f4b8e081 - vkd3d-shader: Implement a function to build a varying map between sm1 stages.

    Compare with previous version

  • +/**
    + * A structure describing the mapping of a source varying (output) register in
    + * a shader stage, to a destination varying (input) register in the following
    + * shader stage.
    + *
    + * This structure is provided as an array, and therefore the source register is
    + * named implicitly by its index in the array.
    + *
    + * This structure is used in struct vkd3d_shader_next_stage_info.
    + */
    +struct vkd3d_shader_varying_map
    +{
    +    /** The index of the destination varying to map this register to. */
    +    unsigned int dst_index;
    +    /** The mask consumed by the destination register. */
    +    unsigned int dst_mask;
    +};

    How do we represent registers not read by the next stage? vkd3d_shader_build_varying_map() seems to suggest we should map them to a free register, but I'm not sure that's ideal. It also sets "dst_mask" to 0, which is perhaps easier to work with. (So far that and the FIXME also seem like the only usage of "dst_mask" though...) In any case, the API documentation here leaves it unspecified.

    Can we split registers? E.g., map o0.xyz in the vertex shader to v0.x and v1.zw in the pixel shader. Is that something we need to be able to do?

    +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 varying registers in this shader stage to input
    +     * varying registers in the next shader stage.
    +     *
    +     * If absent, vkd3d-shader will map registers directly based on their
    +     * register index.
    +     *
    +     * This field should be provied when compiling from legacy Direct3D
    +     * bytecode.
    +     */
    +    const struct vkd3d_shader_varying_map *varying_map;

    "provided"

    Is this strictly necessary for d3dbc shaders? I suppose "should" leaves the possibility of not providing it open, but remap_output_signature() seems to care less about it being absent than "should" may imply. Note also that shader model 1 and 2 d3dbc shaders have a natural mapping between vertex shader outputs and pixel shader inputs.

    +    /**
    +     * The number of registers provided in \ref varying_map.
    +     */
    +    unsigned int varying_map_size;
    +};

    So, "register_count"? :)

    +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;
    +}

    "x" should probably be a uint32_t, although I think there's also an argument for making it uintptr_t sized.

    I don't think we want the #else implementation; it seems preferable to #error out. On Windows we could potentially use _BitScanForward(), and POSIX provides ffs(), but even without either of those we could probably do better in plain C.

    +     * This mapping should be constructed by vkd3d_shader_build_varying_map().

    Should it? If we're going to prescribe that, we might as well just pass the nest stage input signature in struct vkd3d_shader_next_stage_info, and only provide a way to retrieve the output map used, whether that's vkd3d_shader_build_varying_map() or simply vkd3d_shader_scan().

    + * This function should be called twice: once, with an input count of zero, in
    + * order to retrieve the necessary size of the varyings map, and again, with an
    + * input count containing the actual number of varyings to retrieve.
    + *
    + * \remark In practice, since register indices are assigned (from scanning)
    + * according to the actual Direct3D register index, it is safe to assume that
    + * the highest register index for a legacy Direct3D shader is no higher than 11,
    + * and hence one may simply call this function with a pre-allocated array of 12
    + * varyings.

    It might be nice to guarantee an upper bound of output_signature->element_count. That might require representing "varyings" in a way that omits unused registers, but that might not be a bad thing.

    Of course, an alternative way to avoid calling vkd3d_shader_build_varying_map() twice would be to have vkd3d_shader_build_varying_map() allocate the output map itself.

  • Author Developer

    How do we represent registers not read by the next stage? vkd3d_shader_build_varying_map() seems to suggest we should map them to a free register, but I'm not sure that's ideal. It also sets "dst_mask" to 0, which is perhaps easier to work with. (So far that and the FIXME also seem like the only usage of "dst_mask" though...) In any case, the API documentation here leaves it unspecified.

    dst_mask of zero, yes. Mapping them to a free register is done so that the current spirv code outputs them to a free location.Ideally we wouldn't output them at all, but I left that out of scope of the current patch.

    Similarly, we should be outputting only the varyings that the PS uses [unless we have some Vulkan extension I forget the name of], but I left that out of scope of this patch as well. Point is, we have those things in the API so we can use them going forward.

    Sort-of-underspecifying this struct, and how vkd3d_shader_build_varying_map() builds its output, was a deliberate choice, since I wanted to basically say "you should use those functions", and give us a bit of freedom at least as to what registers to actually use.

    Can we split registers? E.g., map o0.xyz in the vertex shader to v0.x and v1.zw in the pixel shader. Is that something we need to be able to do?

    Well, sm4 doesn't need it. As for sm1... I can get the native assembler to output two dcl instructions with the same semantic, but I don't know how that's interpreted. Is that something that you've encountered before?

    I suspect that even if it is, the right thing to do here is handle that in the pixel shader, and convert it to a single semantic. I don't think our current spirv code is set up to handle it otherwise.

    Is this strictly necessary for d3dbc shaders? I suppose "should" leaves the possibility of not providing it open, but remap_output_signature() seems to care less about it being absent than "should" may imply.

    It's not necessary if the registers happen to match, but that is probably unlikely. I wouldn't object to making it a hard requirement for d3dbc.

    Note also that shader model 1 and 2 d3dbc shaders have a natural mapping between vertex shader outputs and pixel shader inputs.

    They do, though there is a snag. 1.x/2.x can output as many as 10 registers (8 texcoord and 2 colour. Also, is fog an issue here?), but early versions of GL might not be able to support that many locations (4.6 requires 16, as does Vulkan, but 3.0 seems to require only 8), which would force us to do remapping—and if the varyings aren't the same, I don't think we can remap in a consistent manner. So we may end up needing this struct for 1.x/2.x as well.

    +    /**
    +     * The number of registers provided in \ref varying_map.
    +     */
    +    unsigned int varying_map_size;
    +};

    So, "register_count"? :)

    Sure, though that means it's not named like varying_map...

    +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;
    +}

    "x" should probably be a uint32_t, although I think there's also an argument for making it uintptr_t sized.

    It was done on the model of wined3d and the other functions here, but sure, I can change it.

    I don't think we want the #else implementation; it seems preferable to #error out. On Windows we could potentially use _BitScanForward(), and POSIX provides ffs(), but even without either of those we could probably do better in plain C.

    Same applies here, though more saliently, I'm not sure how to interpret the statements "I don't think we want the #else implementation" alongside "we could probably do better in plain C"?

    +     * This mapping should be constructed by vkd3d_shader_build_varying_map().

    Should it? If we're going to prescribe that, we might as well just pass the nest stage input signature in struct vkd3d_shader_next_stage_info, and only provide a way to retrieve the output map used, whether that's vkd3d_shader_build_varying_map() or simply vkd3d_shader_scan().

    Well... "should", not "must", but moreover, the whole point here is that passing the signature to struct vkd3d_shader_next_stage_info got complicated in some ways, and this is nicer. Keeping this function around solves the caching problem, but:

    (1) we'd have to reorganize some things in wined3d to allow passing the signature; it currently expects to use only the cache key as compile arguments. Minor problem, I guess, but if we can pass the cache key, I don't see a reason not to.

    (2) this mapping is already kind of in optimal ("compiled", in a sense) form for vkd3d_shader_compile(), since we just look up the mapping in an array.

    It might be nice to guarantee an upper bound of output_signature->element_count. That might require representing "varyings" in a way that omits unused registers, but that might not be a bad thing.

    I originally was going to write that, but decided that since I was indexing the array by register_index, it wouldn't work. But on reflection there's no reason we can't index the array by the signature index, so yeah, that'd be better.

  • Can we split registers? E.g., map o0.xyz in the vertex shader to v0.x and v1.zw in the pixel shader. Is that something we need to be able to do?

    Well, sm4 doesn't need it. As for sm1... I can get the native assembler to output two dcl instructions with the same semantic, but I don't know how that's interpreted. Is that something that you've encountered before?

    I seem to recall seeing applications doing that, yes, though it's also long enough ago that I don't remember for sure.

    Right now, I'm just looking for either "that can't possibly happen" or "we could handle that in this way, if needed". The backup is of course doing a v2 of the structure, but it would be nice to avoid that if we can.

    Is this strictly necessary for d3dbc shaders? I suppose "should" leaves the possibility of not providing it open, but remap_output_signature() seems to care less about it being absent than "should" may imply.

    It's not necessary if the registers happen to match, but that is probably unlikely. I wouldn't object to making it a hard requirement for d3dbc.

    Note also that shader model 1 and 2 d3dbc shaders have a natural mapping between vertex shader outputs and pixel shader inputs.

    They do, though there is a snag. 1.x/2.x can output as many as 10 registers (8 texcoord and 2 colour. Also, is fog an issue here?), but early versions of GL might not be able to support that many locations (4.6 requires 16, as does Vulkan, but 3.0 seems to require only 8), which would force us to do remapping—and if the varyings aren't the same, I don't think we can remap in a consistent manner. So we may end up needing this struct for 1.x/2.x as well.

    Right, a caller may want to remap, but it may also not necessarily need to. Also, does that imply we may need to remap pixel shader inputs as well, along the lines of wined3d_pixel_shader's "input_reg_map"?

    +    /**
    +     * The number of registers provided in \ref varying_map.
    +     */
    +    unsigned int varying_map_size;
    +};

    So, "register_count"? :)

    Sure, though that means it's not named like varying_map...

    We could also say "varying_count", I suppose. The issue I have with using "_size" for fields like these is that it's often ambiguous whether it refers to the size of the allocation or the number of elements.

    +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;
    +}

    "x" should probably be a uint32_t, although I think there's also an argument for making it uintptr_t sized.

    It was done on the model of wined3d and the other functions here, but sure, I can change it.

    Yeah, I figured. The reason wined3d's wined3d_bit_scan() takes an unsigned int is essentially just "history", but it would be nice to make this consistent with the other bitmap functions like bitmap_set() here.

    I don't think we want the #else implementation; it seems preferable to #error out. On Windows we could potentially use _BitScanForward(), and POSIX provides ffs(), but even without either of those we could probably do better in plain C.

    Same applies here, though more saliently, I'm not sure how to interpret the statements "I don't think we want the #else implementation" alongside "we could probably do better in plain C"?

    We seem to have grown the #else path in wined3d_bit_scan() at some point during Wine's PE move, but I'd argue we never want to use it; iterating over all bits is just not a sufficient implementation for something like wined3d_stateblock_apply() or context_apply_draw_state().

    As for doing better than a linear scan without using either POSIX or Win32 functions, plenty has been written about that by other people. One fairly decent approach is to use a de Bruijn sequence together with a small lookup table. Still, I think ffs() and _BitScanForward() should cover everything we care about.

    +     * This mapping should be constructed by vkd3d_shader_build_varying_map().

    Should it? If we're going to prescribe that, we might as well just pass the nest stage input signature in struct vkd3d_shader_next_stage_info, and only provide a way to retrieve the output map used, whether that's vkd3d_shader_build_varying_map() or simply vkd3d_shader_scan().

    Well... "should", not "must",

    I'd argue it's more of a "may".

  • Author Developer

    Can we split registers? E.g., map o0.xyz in the vertex shader to v0.x and v1.zw in the pixel shader. Is that something we need to be able to do?

    Well, sm4 doesn't need it. As for sm1... I can get the native assembler to output two dcl instructions with the same semantic, but I don't know how that's interpreted. Is that something that you've encountered before?

    I seem to recall seeing applications doing that, yes, though it's also long enough ago that I don't remember for sure.

    Right now, I'm just looking for either "that can't possibly happen" or "we could handle that in this way, if needed". The backup is of course doing a v2 of the structure, but it would be nice to avoid that if we can.

    Sure.

    Unless actually doing that does something unexpected, I think it would be covered from an API perspective by emitting multiple dcls as a single entry in the signature (with a combined mask), and then adding code in the spirv to blit to the individual register variables. Actually I think our signature scanning code is already written that way, and depending on how the I/O lowering pass is written, it may be as well.

    Right, a caller may want to remap, but it may also not necessarily need to. Also, does that imply we may need to remap pixel shader inputs as well, along the lines of wined3d_pixel_shader's "input_reg_map"?

    Broadly. The way I was picturing this would actually work would be that you'd pass the varying limit as an argument to v-s (as a compile option?), vs would automatically remap PS registers according to the limit (setting their register_index in the signature accordingly), and then vkd3d_shader_build_varying_map() would use those mapped register indices to build the varying map for the VS.

    +    /**
    +     * The number of registers provided in \ref varying_map.
    +     */
    +    unsigned int varying_map_size;
    +};

    So, "register_count"? :)

    Sure, though that means it's not named like varying_map...

    We could also say "varying_count", I suppose. The issue I have with using "_size" for fields like these is that it's often ambiguous whether it refers to the size of the allocation or the number of elements.

    I think varying_count is close enough, yeah.

    +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;
    +}

    "x" should probably be a uint32_t, although I think there's also an argument for making it uintptr_t sized.

    It was done on the model of wined3d and the other functions here, but sure, I can change it.

    Yeah, I figured. The reason wined3d's wined3d_bit_scan() takes an unsigned int is essentially just "history", but it would be nice to make this consistent with the other bitmap functions like bitmap_set() here.

    I don't think we want the #else implementation; it seems preferable to #error out. On Windows we could potentially use _BitScanForward(), and POSIX provides ffs(), but even without either of those we could probably do better in plain C.

    Same applies here, though more saliently, I'm not sure how to interpret the statements "I don't think we want the #else implementation" alongside "we could probably do better in plain C"?

    We seem to have grown the #else path in wined3d_bit_scan() at some point during Wine's PE move, but I'd argue we never want to use it; iterating over all bits is just not a sufficient implementation for something like wined3d_stateblock_apply() or context_apply_draw_state().

    As for doing better than a linear scan without using either POSIX or Win32 functions, plenty has been written about that by other people. One fairly decent approach is to use a de Bruijn sequence together with a small lookup table. Still, I think ffs() and _BitScanForward() should cover everything we care about.

    Makes sense, thanks.

    +     * This mapping should be constructed by vkd3d_shader_build_varying_map().

    Should it? If we're going to prescribe that, we might as well just pass the nest stage input signature in struct vkd3d_shader_next_stage_info, and only provide a way to retrieve the output map used, whether that's vkd3d_shader_build_varying_map() or simply vkd3d_shader_scan().

    Well... "should", not "must",

    I'd argue it's more of a "may".

    Sure, that's reasonable. I'll make it more clear what the API for this is, then.

  • Author Developer

    It might be nice to guarantee an upper bound of output_signature->element_count. That might require representing "varyings" in a way that omits unused registers, but that might not be a bad thing.

    I originally was going to write that, but decided that since I was indexing the array by register_index, it wouldn't work. But on reflection there's no reason we can't index the array by the signature index, so yeah, that'd be better.

    Hmm, there is one extra snag here: PS inputs not written (at all) by the VS. wined3d cares enough to explicitly zero these, so I assume there are applications that care.

    From the perspective of next_stage_info, I think the simple thing to do is say that varying_map is still indexed by output signature index, but also that it can have extra entries at the end for PS inputs not written by the VS.

    From the perspective of vkd3d_shader_build_varying_map(), that means we can't just say that the size is output_signature->element_count; it might be greater than that. Allocation is possible but I'm a little resistant to that, since in practice the user should just be able to say that sm1 has a maximum inter-stage varying count of 12, provide that array, and call the function once [as my wined3d PoC does].

    Unless you meant to write "an upper bound of input_signature->element_count" [which may make more sense given your second sentence]? That'd work, though it'd be meaner to remap_output_signature().

  • Author Developer

    I originally was going to write that, but decided that since I was indexing the array by register_index, it wouldn't work. But on reflection there's no reason we can't index the array by the signature index, so yeah, that'd be better.

    Ugh, but the annoying thing about doing this is that I/O normalization passes reorder the signature...

  • Unless you meant to write "an upper bound of input_signature->element_count" [which may make more sense given your second sentence]? That'd work, though it'd be meaner to remap_output_signature().

    I think I did. It shouldn't be any meaner than "output_signature->element_count + some extra stuff" though; it's a different bound, but it doesn't say anything about the order by itself. I wouldn't necessarily be opposed to specifying this as e.g. a sorted array of "{src_index, dst_index, dst_mask}" either though.

    I originally was going to write that, but decided that since I was indexing the array by register_index, it wouldn't work. But on reflection there's no reason we can't index the array by the signature index, so yeah, that'd be better.

    Ugh, but the annoying thing about doing this is that I/O normalization passes reorder the signature...

    I think it should only do that temporarily? shader_signature_merge() has a qsort() at the end that should restore the original order. It does of course potentially merge elements, which is perhaps just as bad.

  • Author Developer

    Unless you meant to write "an upper bound of input_signature->element_count" [which may make more sense given your second sentence]? That'd work, though it'd be meaner to remap_output_signature().

    I think I did. It shouldn't be any meaner than "output_signature->element_count + some extra stuff" though; it's a different bound, but it doesn't say anything about the order by itself. I wouldn't necessarily be opposed to specifying this as e.g. a sorted array of "{src_index, dst_index, dst_mask}" either though.

    Well, the current order allows remap_output_signature() just to look up by register_index or by signature index. Either of the above options would instead require us to do a secondary loop. From a performance standpoint that may not meaningfully matter, though; I guess we're going to do that loop once somewhere.

    I originally was going to write that, but decided that since I was indexing the array by register_index, it wouldn't work. But on reflection there's no reason we can't index the array by the signature index, so yeah, that'd be better.

    Ugh, but the annoying thing about doing this is that I/O normalization passes reorder the signature...

    I think it should only do that temporarily? shader_signature_merge() has a qsort() at the end that should restore the original order. It does of course potentially merge elements, which is perhaps just as bad.

    Yeah, in theory it does, but that's not working out for some reason. I don't think I fully investigated why, but if we're messing with the signature order at all then I think we want to remap before doing I/O normalization. Which isn't actually ugly, I just had some amount of tunnel vision there.

  • Elizabeth Figura added 37 commits

    added 37 commits

    • f4b8e081...3d49b59a - 32 commits from branch wine:master
    • f0c3bd87 - vkd3d-shader/d3dbc: Make sure all inter-stage varyings have a unique register index.
    • d395650f - vkd3d-shader: Add a separate field for the target location of a signature element.
    • b5abc699 - vkd3d-shader: Implement remapping shader output registers to match the next shader's semantics.
    • c23f4c1e - vkd3d-shader/spirv: Make output varyings not consumed by the next stage private variables.
    • 4ecb627a - vkd3d-shader: Introduce a function to build a varying map between sm1 stages.

    Compare with previous version

  • added 1 commit

    • f32937d3 - vkd3d-shader: Introduce a function to build a varying map between sm1 stages.

    Compare with previous version

  • Author Developer

    I think I did. It shouldn't be any meaner than "output_signature->element_count + some extra stuff" though; it's a different bound, but it doesn't say anything about the order by itself. I wouldn't necessarily be opposed to specifying this as e.g. a sorted array of "{src_index, dst_index, dst_mask}" either though.

    v4 does this, albeit without the sorting.

    I also went ahead and added a patch to avoid outputting semantics not read by the next shader stage; it ended up being simpler than I was afraid it would be.

  • +    /* Before normalization, this is the register index in the source shader.
    +     * After normalization (or always for DXIL), this is the *signature* index. */
         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;

    It's probably out of scope for this series, but it seems like some renaming is in order here; we probably don't want the meaning of "register_index" to change like that.

    +    if (parser->shader_version.major < 4 && parser->shader_version.type == VKD3D_SHADER_TYPE_VERTEX
    +            && (result = remap_output_signature(&parser->shader_desc.output_signature, compile_info)) < 0)
    +        return result;

    I don't mind too much, but do we need to limit this like that? I can't say it would necessarily be terribly useful, but if someone wanted to remap e.g. sm4 vertex shader outputs, should we try to stop them?

    +        else
    +        {
    +            varyings[count].output_signature_index = unused_signature_idx++;
    +        }

    It's probably inconsequential, but we could just set this to "output_signature->element_count", right?

  • added 1 commit

    • 2cff8914 - vkd3d-shader: Introduce a function to build a varying map between sm1 stages.

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply
Loading