Skip to content
Snippets Groups Projects

vkd3d-shader: Introduce SPIRV_ENVIRONMENT_VULKAN_1_1.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:spirv_1_3 into master
4 unresolved threads

Shader Model 6 wave ops require instructions not available by extension in SPIR-V 1.0, and device support info is found in VkPhysicalDeviceSubgroupProperties, which also has no equivalent by extension in Vulkan 1.0.

Edited by Conor McCarthy

Merge request reports

Merge request pipeline #25349 skipped

Merge request pipeline skipped for b0145ad9

Approval is optional

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (Apr 17, 2024 9:29pm UTC)

Merge details

  • Changes merged into with b0145ad9.
  • Deleted the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
215 215 * This corresponds to the "shaderFloat64" feature in the Vulkan API, and
216 216 * the "GL_ARB_gpu_shader_fp64" extension in the OpenGL API. */
217 217 VKD3D_SHADER_COMPILE_OPTION_FEATURE_FLOAT64 = 0x00000002,
218 /** The SPIR-V target environment supports wave operations.
219 * This corresponds to the following minimum requirements in
220 * VkPhysicalDeviceSubgroupProperties:
221 * subgroupSize >= 4
222 * supportedOperations has BASIC, VOTE, ARITHMETIC, BALLOT, SHUFFLE and
223 * QUAD bits set.
224 * supportedStages include COMPUTE and FRAGMENT. */
225 VKD3D_SHADER_COMPILE_OPTION_FEATURE_WAVE_OPS = 0x00000004,
    • Along something of the same lines as Gio's comment, is VKD3D_SHADER_SPIRV_ENVIRONMENT_VULKAN_1_1 actually doing anything? We need VKD3D_SHADER_COMPILE_OPTION_FEATURE_WAVE_OPS anyway; should we just set the SPIRV version and Vulkan environment based on that?

      Note for review: I did double check, and this functionality is core in Vulkan 1.1 without having gone through an extension first.

    • Along something of the same lines as Gio's comment, is VKD3D_SHADER_SPIRV_ENVIRONMENT_VULKAN_1_1 actually doing anything? We need VKD3D_SHADER_COMPILE_OPTION_FEATURE_WAVE_OPS anyway; should we just set the SPIRV version and Vulkan environment based on that?

      My understanding is that they're somewhat independent features: there might be differences between environments which we want to respect even if they're not captured by the features of the specific device we're using. If we're feeding the SPIR-V code to OpenGL vs Vulkan we might need to generate different code even if it's going to run on the same GPU (thus with the same capabilities). In abstract, at least; I don't know whether we already have an instance of that, but it makes sense for the API to be ready.

    • Sure, but we already have the GL vs Vulkan environment. Is there actually a meaningful difference between Vulkan 1.0 and 1.1 that's not going to end up being an extension or feature bit? And, if we don't know of one yet, do we need to add this extra API switch yet?

    • Author Developer

      should we just set the SPIRV version and Vulkan environment based on that?

      Do you mean, make VULKAN_1_1 be a kind of proxy for wave ops support, i.e. the client checks VkPhysicalDeviceSubgroupProperties for wave op support, and uses VULKAN_1_1 only if they are available?

    • I think the other way around; we keep VKD3D_SHADER_COMPILE_OPTION_FEATURE_WAVE_OPS but remove VKD3D_SHADER_SPIRV_ENVIRONMENT_VULKAN_1_1, and wherever we check the environment in the code we check for wave ops instead.

      Not a big deal though and I can see just keep it around for some measure of consistency.

    • Sure, but we already have the GL vs Vulkan environment. Is there actually a meaningful difference between Vulkan 1.0 and 1.1 that's not going to end up being an extension or feature bit? And, if we don't know of one yet, do we need to add this extra API switch yet?

      I don't understand, are we adding an API switch? It seems that we're only adding a case to an API that already exists (and is the one that enables the SPIR-V writer to distinguish between OpenGL and Vulkan 1.0; now we also have Vulkan 1.1). My feeling is that since SPIR-V already has a concept of "environment" we should stick to that one instead of inventing our own, so that anybody already knowing SPIR-V finds it a familiar concept instead of having to learn something similar-but-not-completely-the-same which might be puzzling. Also, coming up with a different concept might mean that at some point the SPIR-V concept and ours diverge in some sense that requires us to change our API to something more unwieldy.

    • Please register or sign in to reply
    • Should there be a rule mandating than when you're compiling with this flag you should also specify VKD3D_SHADER_SPIRV_ENVIRONMENT_VULKAN_1_1 as the target environment?

      I don't think we need to explicitly enforce this in libvkd3d-shader, but it's probably worth pointing out in the documentation.

      I don't think the MR does a great job explaining the issue it addresses, so I spent a bunch of time read through the various specs, but for reference:

      • Vulkan 1.0 supports SPIR-V 1.0.
      • Vulkan 1.1 supports SPIR-V 1.0, 1.1, 1.2, and 1.3.
      • GL_ARB_gl_spirv supports SPIR-V 1.0.
      • SPIR-V 1.3 doesn't incorporate any of the existing vkd3d_shader_spirv_extension extensions.
      • There exist extensions like SPV_KHR_shader_ballot that bring some subgroup/wave operations to SPIR-V 1.0, but (AFAIK) not for all of the operations required for D3D shader model 6 wave operations. That requires SPIR-V 1.3 along with the corresponding VkPhysicalDeviceSubgroupProperties features, and by implication Vulkan 1.1.

      The implication is that in theory you could set VKD3D_SHADER_COMPILE_OPTION_FEATURE_WAVE_OPS with VKD3D_SHADER_SPIRV_ENVIRONMENT_VULKAN_1_0, but that wouldn't allow translation of sm6 waveops because we wouldn't be able to use SPIR-V 1.3.

    • Author Developer

      There exist extensions like SPV_KHR_shader_ballot that bring some subgroup/wave operations to SPIR-V 1.0, but (AFAIK) not for all of the operations required for D3D shader model 6 wave operations.

      That was my conclusion. The extensions don't supply equivalents for all of the DX intrinsics needed for 6.0 support.

    • Please register or sign in to reply
  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Conor McCarthy added 1 commit

    added 1 commit

    • 6580c5de - vkd3d-shader: Introduce a wave ops feature flag.

    Compare with previous version

  • The implication is that in theory you could set VKD3D_SHADER_COMPILE_OPTION_FEATURE_WAVE_OPS with VKD3D_SHADER_SPIRV_ENVIRONMENT_VULKAN_1_0, but that wouldn't allow translation of sm6 waveops because we wouldn't be able to use SPIR-V 1.3.

    Part of what I'm thinking is, from my interpretation of the spec, it's not actually legal for an implementation to expose all the flags we need for wave ops but not actually support 1.1 (and by extension SPIR-V 1.3). So adding anything about 1.1 is kind of redundant.

    Having VKD3D_SHADER_SPIRV_ENVIRONMENT_VULKAN_1_1 makes sense for some measure of consistency; it's just not clear to me we need it.

  • Conor McCarthy added 3 commits

    added 3 commits

    • b52f88cc - vkd3d-shader: Introduce SPIRV_ENVIRONMENT_VULKAN_1_1.
    • fcc0ebad - vkd3d: Use Vulkan 1.1 if available.
    • af539d7e - vkd3d-shader: Introduce a wave ops feature flag.

    Compare with previous version

    • Part of what I'm thinking is, from my interpretation of the spec, it's not actually legal for an implementation to expose all the flags we need for wave ops but not actually support 1.1 (and by extension SPIR-V 1.3). So adding anything about 1.1 is kind of redundant.

      Yes, kind of. In principle e.g. an OpenGL extension could expose support for SPIR-V 1.3 in OpenGL environments though.

      Having VKD3D_SHADER_SPIRV_ENVIRONMENT_VULKAN_1_1 makes sense for some measure of consistency; it's just not clear to me we need it.

      It's probably not strictly necessary, but it doesn't seem worse than the alternative to me, at least. For example, it seems potentially unintuitive that FEATURE_WAVE_OPS would cause SPIR-V 1.3 to be generated instead of SPIR-V 1.0.

      @@ -923,6 +923,7 @@ enum vkd3d_shader_spirv_environment
           VKD3D_SHADER_SPIRV_ENVIRONMENT_NONE,
           VKD3D_SHADER_SPIRV_ENVIRONMENT_OPENGL_4_5,
           VKD3D_SHADER_SPIRV_ENVIRONMENT_VULKAN_1_0, /* default target */
      +    VKD3D_SHADER_SPIRV_ENVIRONMENT_VULKAN_1_1,
       
           VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_SPIRV_ENVIRONMENT),
       };

      This will need a \since.

      +    /** The SPIR-V target environment supports wave operations.
      +     * This flag is valid only in VKD3D_SHADER_SPIRV_ENVIRONMENT_VULKAN_1_1
      +     * or greater, and corresponds to the following minimum requirements in
      +     * VkPhysicalDeviceSubgroupProperties:
      +     * - subgroupSize >= 4.
      +     * - supportedOperations has BASIC, VOTE, ARITHMETIC, BALLOT, SHUFFLE and
      +     *       QUAD bits set.
      +     * - supportedStages include COMPUTE and FRAGMENT. */
      +    VKD3D_SHADER_COMPILE_OPTION_FEATURE_WAVE_OPS      = 0x00000004,

      Likewise.

      +    /* vkEnumerateInstanceVersion was added in Vulkan 1.1, and its absence indicates only 1.0 is supported. */
      +    vkEnumerateInstanceVersion = (void *)vk_global_procs->vkGetInstanceProcAddr(NULL, "vkEnumerateInstanceVersion");
      +    if (vkEnumerateInstanceVersion && vkEnumerateInstanceVersion(&vk_api_version) >= 0
      +            && vk_api_version >= VK_API_VERSION_1_1)
      +    {
      +        TRACE("Vulkan API version 1.1 is available; requesting it.\n");
      +        application_info.apiVersion = VK_API_VERSION_1_1;
      +    }
      +    instance->vk_api_version = application_info.apiVersion;

      Is 1.1 fully backwards compatible with 1.0, or should this be behind a VKD3D_API_VERSION check?

    • Author Developer

      Having VKD3D_SHADER_SPIRV_ENVIRONMENT_VULKAN_1_1 makes sense for some measure of consistency; it's just not clear to me we need it.

      One complication may make this moot: QuadReadLaneAt supports any uniform constant for the lane index, but SPIR-V 1.3 requires an immediate constant index for OpGroupNonUniformQuadBroadcast. If this turns up in games, we would need SPIR-V 1.5 and Vulkan 1.2, so the compiler would need to know which is available.

      Is 1.1 fully backwards compatible with 1.0, or should this be behind a VKD3D_API_VERSION check?

      The spec says:

      Specifications with a lower minor version are backwards compatible with an implementation of a specification with a higher minor version for core functionality and extensions issued with the KHR vendor tag. Vendor and multi-vendor extensions are not guaranteed to remain functional across minor versions, though in general they are with few exceptions

      I checked the extensions, and the info on Vulkan 1.1 in the spec appendix C, and am not aware of any incompatibilities.

    • Please register or sign in to reply
  • Conor McCarthy added 3 commits

    added 3 commits

    • e24609c0 - vkd3d-shader: Introduce SPIRV_ENVIRONMENT_VULKAN_1_1.
    • 4bbd8b7e - vkd3d: Use Vulkan 1.1 if available.
    • 726c7616 - vkd3d-shader: Introduce a wave ops feature flag.

    Compare with previous version

  • Conor McCarthy added 75 commits

    added 75 commits

    • 726c7616...1d6c3eae - 72 commits from branch wine:master
    • dafafd5e - vkd3d-shader: Introduce SPIRV_ENVIRONMENT_VULKAN_1_1.
    • e4b0ee8c - vkd3d: Use Vulkan 1.1 if available.
    • bd53a3bc - vkd3d-shader: Introduce a wave ops feature flag.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • added 9 commits

    • bd53a3bc...a7870e17 - 6 commits from branch wine:master
    • 6975a8d7 - vkd3d-shader: Introduce SPIRV_ENVIRONMENT_VULKAN_1_1.
    • 270aa22d - vkd3d: Use Vulkan 1.1 if available.
    • b0145ad9 - vkd3d-shader: Introduce a wave ops feature flag.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading