Skip to content
Snippets Groups Projects

vkd3d-shader: sm1 constants.

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

Most of the interesting part is the API (first patch); the rest is implementation. The basic idea is that we have predefined fixed numbers for the float, integer, and boolean constant arrays, and treat each of the three like a constant buffer.

Implementation-wise, we lower sm1-style constant registers to sm4-style constant buffers and immediate constants, which are capable of expressing a strict superset of the same functionality.

Note that use of immediate constants is not the way that wined3d currently handles DEF instructions, but rather those are uploaded via uniforms. Is there a reason for this?

Relative addressing is not yet implemented. In theory it should be simple enough to either translate it directly, for external constants, or use sm4-style immediate constant buffers, for internal constants. There may be a snag if an application depends on relative addressing to cover a range of both internal and external constants; this surely would require manual assembly or an application bug, but we could implement it by copying to a temporary array using a private TEMP-like register type.

--

Because API is easiest to review when there's a concrete user, I have functional patches hooking this up to wined3d, in the following branches:

https://gitlab.winehq.org/zfigura/vkd3d/-/commits/himavant5

https://gitlab.winehq.org/zfigura/wine/-/commits/himavant_cb

The vkd3d branch contains some extra commits necessary to compile basic shaders; I intend to submit these as soon as possible after the important API parts have been reviewed. I tested this patch series by building a mingw vkd3d tree with that branch, and running the shader runner through Wine, with a test that uses both internal and external constants:

make tests/shader_runner.exe && WINE_D3D_CONFIG=renderer=vulkan wine tests/shader_runner.exe ../vkd3d/tests/hlsl/writemask-assignop-0.shader_test

I actually originally wrote the API without a user in mind, and later hooked up the implementation in Wine, and was surprised to find how straightforward it ended up being, so I think that speaks quite strongly in favour of this API. Granted, I had already written "wined3d: Store push constants in wined3d_buffers in struct wined3d_state." by that point, but that was something I anticipated needing for Wine anyway, without thinking of vkd3d.

--

I actually originally did the implementation all in spirv.c, mostly because this was before we had infrastructure in place to do passes on the middle IR. I much prefer this version, it's quite centralized in one place and I think ends up being simpler than the spirv.c version anyway, but I can retrieve the spirv.c version if anyone wants to see it.

That said, we may not want to lower to VKD3DSPR_CONSTBUFFER for GLSL without UBOs (but then again, we could also just emit VKD3DSPR_CONSTBUFFER registers as plain GLSL arrays).

The actual declaration of flat constants is kept in spirv. I suppose the alternative here is to instead declare buffers from the reflection information and simply ignore dcl_constantbuffer. I submitted the patch as-is since it seemed simple enough and I didn't want to block this work further on rewriting that part, but we may want to rewrite it in the future regardless.

    After the torchlight red on sweaty faces
    After the frosty silence in the gardens
    After the agony in stony places
    The shouting and the crying
    Prison and palace and reverberation
    Of thunder of spring over distant mountains

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
  • Note that use of immediate constants is not the way that wined3d currently handles DEF instructions, but rather those are uploaded via uniforms. Is there a reason for this?

    We do use immediates when we can, see the "if (!shader->load_local_constsF)" block at the end of shader_generate_glsl_declarations(). There are a couple of cases where we can't use immediates though; relative addressing of constants is one case, and having non-finite values without GL_ARB_shader_bit_encoding is another.

    It's perhaps also worth pointing out that Direct3D 8 can load shader constants from the vertex declaration using "D3DVSD_CONST"; we have a couple of examples of that in the Wine d3d8 tests.

    Because API is easiest to review when there's a concrete user, I have functional patches hooking this up to wined3d, in the following branches:

    https://gitlab.winehq.org/zfigura/vkd3d/-/commits/himavant5

    https://gitlab.winehq.org/zfigura/wine/-/commits/himavant_cb

    "wined3d: Compile sm1 bytecode to spirv." isn't quite right; determining the source type is slightly more complicated because of things like Aon9. See also shader_init(); my "glsl-vkd3d" series just stores "source_type" from that function in struct wined3d_shader.

    That said, we may not want to lower to VKD3DSPR_CONSTBUFFER for GLSL without UBOs (but then again, we could also just emit VKD3DSPR_CONSTBUFFER registers as plain GLSL arrays).

    I imagine the latter is what we'd want to do, yes. I.e., support translating CBVs to GLSL uniform arrays.

    +/**
    + * Symbolic register indices for mapping uniform constants in legacy Direct3D
    + * bytecode to uniform buffers in the target environment.
    + *
    + * Members of this enumeration are used in
    + * \ref vkd3d_shader_resource_binding.register_index.
    + */
    +enum vkd3d_shader_d3dbc_constant_register
    +{
    +    /** The float constant register set, c# in Direct3D assembly. */
    +    VKD3D_SHADER_D3DBC_FLOAT_CONSTANT_REGISTER  = 0x0,
    +    /** The integer constant register set, i# in Direct3D assembly. */
    +    VKD3D_SHADER_D3DBC_INT_CONSTANT_REGISTER    = 0x1,
    +    /** The boolean constant register set, b# in Direct3D assembly. */
    +    VKD3D_SHADER_D3DBC_BOOL_CONSTANT_REGISTER   = 0x2,
    +};

    Seems reasonable enough. We'll probably want a \since as well. For better or worse, the terminology we use in vkd3d-shader is "constant buffer view" instead of "uniform buffers".

    +    /**
    +     * Register index of the Direct3D resource.
    +     *
    +     * When mapping legacy Direct3D constants to constant buffers in the target
    +     * environment, this parameter instead names the register set to map, and
    +     * must be a member of enum vkd3d_shader_d3dbc_constant_register.
    +     */

    The clarification here seems helpful, but I'm not sure I agree with "instead". From the point of view of struct vkd3d_shader_resource_binding this is consistent with the existing usage of the structure; we map the different d3dbc constant files to CBVs, and then simply specify the register indices of those CBVs here.

    + * When scanning a legacy Direct3D shader, vkd3d-shader enumerates each
    + * constant register set used by the shader as a single constant buffer
    + * descriptor, as follows:
    + * - The \ref vkd3d_shader_descriptor_info.type field is set to
    + *   VKD3D_SHADER_DESCRIPTOR_TYPE_CBV.
    + * - The \ref vkd3d_shader_descriptor_info.register_space field is set to zero.
    + * - The \ref vkd3d_shader_descriptor_info.register_index field is set to a
    + *   member of enum vkd3d_shader_d3dbc_constant_register denoting which set
    + *   is used.
    + * - The \ref vkd3d_shader_descriptor_info.resource_type field is set to
    + *   VKD3D_SHADER_RESOURCE_BUFFER.
    + * - The \ref vkd3d_shader_descriptor_info.resource_data_type field is set to
    + *   VKD3D_SHADER_RESOURCE_DATA_FLOAT.
    + * - The \ref vkd3d_shader_descriptor_info.flags field is set to zero.
    + * - The \ref vkd3d_shader_descriptor_info.count field is set to one.
    + *
    + * In summary, there may be up to three such descriptors, one for each register
    + * set used by the shader: float, integer, and boolean.

    Is there a particular reason for going for VKD3D_SHADER_RESOURCE_DATA_FLOAT here? In principle "resource_data_type" is meaningless for CBVs, but we currently use VKD3D_SHADER_RESOURCE_DATA_UINT in vkd3d_shader_scan_constant_buffer_declaration(). One consequence is that we'd get different results if we ran scan_with_parser() after running instruction_array_normalise_flat_constants().

    +static void record_constant_register(struct vkd3d_shader_sm1_parser *sm1,
    +        enum vkd3d_shader_d3dbc_constant_register set, uint32_t index, bool from_def)
     {
    +    struct vkd3d_shader_desc *desc = &sm1->p.shader_desc;
    +
    +    desc->flat_constant_count[set].used = max(desc->flat_constant_count[set].used, index + 1);
    +    if (from_def)
    +    {
    +        assert((index / 32) <= ARRAY_SIZE(sm1->constant_def_mask[set]));
    +        bitmap_set(sm1->constant_def_mask[set], index);
    +    }
    +}

    I think that assert is right, but it's perhaps also obscure enough to warrant a comment. (The maximum d3dbc register index is 0x7ff; there are 4 float constant files, giving a maximum of 8192 float constants.)

  • Author Developer

    Note that use of immediate constants is not the way that wined3d currently handles DEF instructions, but rather those are uploaded via uniforms. Is there a reason for this?

    We do use immediates when we can, see the "if (!shader->load_local_constsF)" block at the end of shader_generate_glsl_declarations().

    Ah, I was looking for DEF handling and didn't see that.

    There are a couple of cases where we can't use immediates though; relative addressing of constants is one case, and having non-finite values without GL_ARB_shader_bit_encoding is another.

    The non-finite values part makes sense to me, but I don't see how relative addressing would require uniforms: is it not possible in GLSL to declare a const array of values and dynamically index that? Or are there actually applications that require dynamically indexing such that we need to return both internal and external constants?

    It's perhaps also worth pointing out that Direct3D 8 can load shader constants from the vertex declaration using "D3DVSD_CONST"; we have a couple of examples of that in the Wine d3d8 tests.

    Is there any reason we can't just implement that on top of wined3d_stateblock_set_vs_consts_f() these days?

    "wined3d: Compile sm1 bytecode to spirv." isn't quite right; determining the source type is slightly more complicated because of things like Aon9. See also shader_init(); my "glsl-vkd3d" series just stores "source_type" from that function in struct wined3d_shader.

    I wasn't too enamoured of that patch either; I'd rather see v-s do the job for us.

    +    /**
    +     * Register index of the Direct3D resource.
    +     *
    +     * When mapping legacy Direct3D constants to constant buffers in the target
    +     * environment, this parameter instead names the register set to map, and
    +     * must be a member of enum vkd3d_shader_d3dbc_constant_register.
    +     */

    The clarification here seems helpful, but I'm not sure I agree with "instead". From the point of view of struct vkd3d_shader_resource_binding this is consistent with the existing usage of the structure; we map the different d3dbc constant files to CBVs, and then simply specify the register indices of those CBVs here.

    It's a CBV slot, yeah, and it makes a lot of sense that way. But the name of the parameter is register_index, and in sm4 it always is a register index (one of two in the case of constant buffers, perhaps), whereas in sm1 the distinction between "b*" / "c*" / "i*" is not one of register index; it is rather a distinction of register set.

    Saying that this describes a "register index", for someone familiar with sm1, would be confusing; it'd imply the distinction between "c0" and "c1", which it doesn't. Changing the documentation to eschew the term "register index" in favor of something more neutral like "CBV slot" might help, but the parameter name is still register_index. I want to be clear about what this actually is, so that someone setting up their bindings knows exactly which binding they're setting up.

    + * When scanning a legacy Direct3D shader, vkd3d-shader enumerates each
    + * constant register set used by the shader as a single constant buffer
    + * descriptor, as follows:
    + * - The \ref vkd3d_shader_descriptor_info.type field is set to
    + *   VKD3D_SHADER_DESCRIPTOR_TYPE_CBV.
    + * - The \ref vkd3d_shader_descriptor_info.register_space field is set to zero.
    + * - The \ref vkd3d_shader_descriptor_info.register_index field is set to a
    + *   member of enum vkd3d_shader_d3dbc_constant_register denoting which set
    + *   is used.
    + * - The \ref vkd3d_shader_descriptor_info.resource_type field is set to
    + *   VKD3D_SHADER_RESOURCE_BUFFER.
    + * - The \ref vkd3d_shader_descriptor_info.resource_data_type field is set to
    + *   VKD3D_SHADER_RESOURCE_DATA_FLOAT.
    + * - The \ref vkd3d_shader_descriptor_info.flags field is set to zero.
    + * - The \ref vkd3d_shader_descriptor_info.count field is set to one.
    + *
    + * In summary, there may be up to three such descriptors, one for each register
    + * set used by the shader: float, integer, and boolean.

    Is there a particular reason for going for VKD3D_SHADER_RESOURCE_DATA_FLOAT here? In principle "resource_data_type" is meaningless for CBVs, but we currently use VKD3D_SHADER_RESOURCE_DATA_UINT in vkd3d_shader_scan_constant_buffer_declaration(). One consequence is that we'd get different results if we ran scan_with_parser() after running instruction_array_normalise_flat_constants().

    No, this was a holdover from an early version that I meant to correct but never did.

  • There are a couple of cases where we can't use immediates though; relative addressing of constants is one case, and having non-finite values without GL_ARB_shader_bit_encoding is another.

    The non-finite values part makes sense to me, but I don't see how relative addressing would require uniforms: is it not possible in GLSL to declare a const array of values and dynamically index that? Or are there actually applications that require dynamically indexing such that we need to return both internal and external constants?

    As far as I know nothing prevents an application from mixing these, and it was not uncommon for shader model 1-3 shaders to be written in assembler, so "the HLSL compiler never generates mixed accesses" wouldn't generally be sufficient to stop us from worrying about them.

    It's perhaps also worth pointing out that Direct3D 8 can load shader constants from the vertex declaration using "D3DVSD_CONST"; we have a couple of examples of that in the Wine d3d8 tests.

    Is there any reason we can't just implement that on top of wined3d_stateblock_set_vs_consts_f() these days?

    It might be possible. The tricky part is probably mostly that D3DVSD_CONST constants override those set by SetVertexShaderConstant(), but we should still need to keep track of constants set by SetVertexShaderConstant() for when we switch vertex shaders.

    +    /**
    +     * Register index of the Direct3D resource.
    +     *
    +     * When mapping legacy Direct3D constants to constant buffers in the target
    +     * environment, this parameter instead names the register set to map, and
    +     * must be a member of enum vkd3d_shader_d3dbc_constant_register.
    +     */

    The clarification here seems helpful, but I'm not sure I agree with "instead". From the point of view of struct vkd3d_shader_resource_binding this is consistent with the existing usage of the structure; we map the different d3dbc constant files to CBVs, and then simply specify the register indices of those CBVs here.

    It's a CBV slot, yeah, and it makes a lot of sense that way. But the name of the parameter is register_index, and in sm4 it always is a register index (one of two in the case of constant buffers, perhaps), whereas in sm1 the distinction between "b*" / "c*" / "i*" is not one of register index; it is rather a distinction of register set.

    Saying that this describes a "register index", for someone familiar with sm1, would be confusing; it'd imply the distinction between "c0" and "c1", which it doesn't. Changing the documentation to eschew the term "register index" in favor of something more neutral like "CBV slot" might help, but the parameter name is still register_index. I want to be clear about what this actually is, so that someone setting up their bindings knows exactly which binding they're setting up.

    Right, as I said, the clarification is probably helpful. I'd just prefer phrasing it as something along the lines of "Note that vkd3d-shader maps constant registers in d3dbc sources to CBV registers in the following way: ..., so use enum vkd3d_shader_d3dbc_constant_register here for those.", in order to avoid (seemingly) implying that the "register_index" field works in a fundamentally different way for d3dbc sources.

  • Elizabeth Figura added 5 commits

    added 5 commits

    • b827ced9 - include: Define an API for d3dbc constants.
    • cb44054d - vkd3d-shader/d3dbc: Scan descriptors for constant register sets.
    • 1a6a143b - vkd3d-shader/ir: Move normalization code from spirv.c to ir.c.
    • c0a22974 - vkd3d-shader/ir: Normalise sm1-style constants.
    • f7688a24 - vkd3d-shader/spirv: Emit variables for flat constant buffers.

    Compare with previous version

  • Author Developer

    There are a couple of cases where we can't use immediates though; relative addressing of constants is one case, and having non-finite values without GL_ARB_shader_bit_encoding is another.

    The non-finite values part makes sense to me, but I don't see how relative addressing would require uniforms: is it not possible in GLSL to declare a const array of values and dynamically index that? Or are there actually applications that require dynamically indexing such that we need to return both internal and external constants?

    As far as I know nothing prevents an application from mixing these, and it was not uncommon for shader model 1-3 shaders to be written in assembler, so "the HLSL compiler never generates mixed accesses" wouldn't generally be sufficient to stop us from worrying about them.

    Makes sense. Although presumably that doesn't mean we have to upload them that way—I was envisioning solving that particular problem by copying both external and internal constants into a temp array in the compiled shader. (But maybe that's more overhead for the lower level compiler?)

    It's perhaps also worth pointing out that Direct3D 8 can load shader constants from the vertex declaration using "D3DVSD_CONST"; we have a couple of examples of that in the Wine d3d8 tests.

    Is there any reason we can't just implement that on top of wined3d_stateblock_set_vs_consts_f() these days?

    It might be possible. The tricky part is probably mostly that D3DVSD_CONST constants override those set by SetVertexShaderConstant(), but we should still need to keep track of constants set by SetVertexShaderConstant() for when we switch vertex shaders.

    I actually said wined3d_stateblock_set_vs_consts_f() but meant wined3d_device_set_vs_consts_f(). Point is, I was envisioning handling it in wined3d_device_apply_stateblock().

    Right, as I said, the clarification is probably helpful. I'd just prefer phrasing it as something along the lines of "Note that vkd3d-shader maps constant registers in d3dbc sources to CBV registers in the following way: ..., so use enum vkd3d_shader_d3dbc_constant_register here for those.", in order to avoid (seemingly) implying that the "register_index" field works in a fundamentally different way for d3dbc sources.

    I think I see what you're trying to get at. I've rewritten this bit in v2; does this seem more reasonable?

  • Makes sense. Although presumably that doesn't mean we have to upload them that way—I was envisioning solving that particular problem by copying both external and internal constants into a temp array in the compiled shader. (But maybe that's more overhead for the lower level compiler?)

    I imagine it would be slower, yes. In part because relative addressing typically means all constants are potentially accessed instead of the more limited set typically used by applications. So we'd introduce a bunch of loads from the UBO/CBV that may never end up actually being used.

    I guess you're trying to avoid having to use vkd3d_shader_scan() to determine the values of "local" constants? It would be nice if we could, but there may not be a good solution for that.

    I think I see what you're trying to get at. I've rewritten this bit in v2; does this seem more reasonable?

    Yeah, that works for me.

    +    for (i = 0; i < ARRAY_SIZE(parser->shader_desc.flat_constant_count); ++i)
    +    {
    +        struct vkd3d_shader_register_range range = {.space = 0, .first = i, .last = i};
    +
    +        if (parser->shader_desc.flat_constant_count[i].external)
    +            vkd3d_shader_scan_add_descriptor(&context, VKD3D_SHADER_DESCRIPTOR_TYPE_CBV,
    +                    &range, VKD3D_SHADER_RESOURCE_BUFFER, VKD3D_SHADER_RESOURCE_DATA_FLOAT, 0);
    +    }

    It's fairly minor, but I'd still feel better about this if it used VKD3D_SHADER_RESOURCE_DATA_UINT. Or perhaps we can use a common helper shared with vkd3d_shader_scan_constant_buffer_declaration().

  • Elizabeth Figura added 4 commits

    added 4 commits

    • b6d01ac5 - vkd3d-shader/d3dbc: Scan descriptors for constant register sets.
    • e4845577 - vkd3d-shader/ir: Move normalization code from spirv.c to ir.c.
    • 2f2f151c - vkd3d-shader/ir: Normalise sm1-style constants.
    • 5cff2470 - vkd3d-shader/spirv: Emit variables for flat constant buffers.

    Compare with previous version

  • Author Developer

    I guess you're trying to avoid having to use vkd3d_shader_scan() to determine the values of "local" constants? It would be nice if we could, but there may not be a good solution for that.

    Pretty much, just trying to avoid more API changes :-)

    It's fairly minor, but I'd still feel better about this if it used VKD3D_SHADER_RESOURCE_DATA_UINT. Or perhaps we can use a common helper shared with vkd3d_shader_scan_constant_buffer_declaration().

    Ah, for consistency with sm4 constant buffers. Fixed in v3.

  • Elizabeth Figura added 4 commits

    added 4 commits

    • 3ec6cb0f - vkd3d-shader/d3dbc: Scan descriptors for constant register sets.
    • 3b1769f9 - vkd3d-shader/ir: Move normalization code from spirv.c to ir.c.
    • d78e27e1 - vkd3d-shader/ir: Normalise sm1-style constants.
    • 94f98fea - vkd3d-shader/spirv: Emit variables for flat constant buffers.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard added 32 commits

    added 32 commits

    • 94f98fea...a18ace73 - 27 commits from branch wine:master
    • 55c5129a - include: Define an API for d3dbc constants.
    • d077562f - vkd3d-shader/d3dbc: Scan descriptors for constant register sets.
    • e9fb067d - vkd3d-shader/ir: Move normalization code from spirv.c to ir.c.
    • 25cf6a72 - vkd3d-shader/ir: Normalise sm1-style constants.
    • e0e261ea - vkd3d-shader/spirv: Emit variables for flat constant buffers.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading