Skip to content
Snippets Groups Projects

vkd3d-shader: Prepare for 64-bit integer support.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:shader_int64 into master

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
191 191 runner->compile_options |= options[i].option;
192 192 }
193 193 }
194 else if (match_string(line, "shader int64", &line))
  • @etang-cw proposed a more flexible syntax for this kind of needs, which I would be more inclined to adopt, in !416 (closed) (specifically !416 (3eff20fd)). That's probably something on which @zfigura and @fcasas might have something to say.

  • Author Developer

    It will be awhile before this goes upstream so I'll see what develops there.

  • I'm inclined to prefer this patch, it's far simpler. We don't need a second level.

    I'd even get rid of the word "shader"; it's rather redundant.

  • I like of Evan's solution that it's similar to the options: syntax that we already have. And yes, it requires some boilerplate code the first time, but then every further option is just an array item.

    However, it's not such a strong opinion. I can live with Zeb's proposal.

  • I like of Evan's solution that it's similar to the options: syntax that we already have. And yes, it requires some boilerplate code the first time, but then every further option is just an array item.

    However, it's not such a strong opinion. I can live with Zeb's proposal.

    Yeah, though if I'd thought about it I would have said the same thing there (or possibly I thought about it but didn't want to bother bikeshedding.)

  • Conor McCarthy changed this line in version 5 of the diff

    changed this line in version 5 of the diff

  • Please register or sign in to reply
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on commit a55ede5d
  • 90 90 uniform 0 float4 1.0 0.0 0.0 0.0
    91 91 draw quad
    92 92 probe all rgba (1e99, 1e99, 1e99, 1e99)
    93
    94 [require]
    95 shader model >= 5.0
    96
    97 [pixel shader todo]
    98 uniform double2 a;
    99
    100 float4 main() : SV_TARGET
    101 {
    102 double x = a.x;
    103 double y = a.y;
    104 return float4(x + y, x - y, x * y, x / y);
    • This is fine for me, but here and below asuint() with a uint4 return type could be used to return an actual double. I don't know if SM6 has something similar for 64 bit integers, but it can be emulated pretty easily with bitshifts anyway.

    • Author Developer

      It has a SplitDouble intrinsic, which I'll probably upstream soon after the release. In shader runner we would need a way to check double value readbacks or at least uints. Maybe we'll need that eventually but I think it's not justified for this.

    • Yeah, I'd say that it makes sense to be able to check double readbacks anyway. I agree it's not required for this.

    • Please register or sign in to reply
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit 19257583
  • 384 385 {
    385 386 VKD3D_SHADER_PARAMETER_NAME_UNKNOWN,
    386 387 VKD3D_SHADER_PARAMETER_NAME_RASTERIZER_SAMPLE_COUNT,
    388 VKD3D_SHADER_PARAMETER_NAME_CAPABILITY_INT64,
    • It's not completely obvious to me that a parameter is what is appropriate here (a parameter would, for instance, allow that data item to be a specialization constant, which does not make any sense here). Even if this is not technically a SPIR-V extension, it seems that the extensions array is more appropriate for defining which features are allowed by the execution environment. It's not even clear to me whether this is something that should be explicitly enabled: all the other SPIR-V capabilities do not require explicit user confirmation for vkd3d-shader to use them, I can't see why int64 is different. And, after all, if a shader requires it there is little that can be done: we either enable it or fail; so we could directly assume it's available and generate a shader, and the execution environment will then decide whether that shader is usable or not (though, to be honest, the same could be told of EXT_DESCRIPTOR_INDEXING and EXT_STENCIL_EXPORT). Accepting an input parameter mostly seems to make sense when we can generate different code depending on whether some feature is available or not (as for EXT_DEMOTE_TO_HELPER_INVOCATION), which could even theoretically be the case for int64, but I don't think there is currently any intention to provide an alternative int64 implementation on top of int32.

    • Author Developer

      And, after all, if a shader requires it there is little that can be done: we either enable it or fail; so we could directly assume it's available and generate a shader, and the execution environment will then decide whether that shader is usable or not

      One problem with this approach is Vulkan pipeline creation functions do not return errors; if they reject a SPIR-V binary they assert, so all the user sees is a crash. I'm not sure what the drivers will do if a binary uses 64-bit ints and they are not supported by the hardware, but crashing is one possibility.

    • Yeah, that's a sensible concern. At any rate, my feeling is that despite the name extensions is better suited for passing this kind of information.

    • Author Developer

      For features, not extensions, which match up with a d3d12 global flag, it may be better for vkd3d-shader to send these to the client as a set of flags in a new scan struct, so the client can determine if a particular shader is supported before attempting to include it in a pipeline build. This places responsibility on the client not to compile unsupported shaders, but that is true of graphics libraries generally.

      In addition to int64, this would cover wave ops, native 16-bit, int64 atomics and probably others, and new additions would only require updating the flag enum, instead of passing yet another extension or parameter and reading it in the backend.

    • I had though about this too, but I'm not sure of what would be the advantage. IIUC what you're proposing is that, instead of having the client declare which features are supported by the environment, we have vkd3d-shader declare which features were used for compiling a particular shader, and leave it to the client to drop the compiled shader if it doesn't support some of the required feature. However the first model seems more useful to me:

      • if a feature is not known by the client but needed by the shader, in the first case compilation will fail, because the client will not declare it; in the second case compilation will succeed, and the client might fail to notice that that feature is required, because it doesn't know about it; this seems problematic;
      • it might happen that a feature is not strictly speaking required, because vkd3d-shader can emulate it with other instructions: in this case the first model vkd3d-shader is aware that the feature is not available and can fall back to emulation code, while in the second model vkd3d-shader doesn't know anything and can only use that feature, only for the shader to be dropped immediately after because the feature is not available.

      In both case supporting new features amounts to adding a flag to an enum, doesn't it?

    • Author Developer

      Another problem with checking features in the client is it requires a shader scan in state.c which is currently skipped if descriptor indexing is fully supported.

      Either way, I really want feature information to be passed as a flag enum. Extensions and parameters are both pretty clunky ways of doing it.

    • I don't know, I don't find them particularly clunky. Your current solution is pretty sensible, except that using extensions make more sense to me rather than parameters (which should even make the code slightly simpler).

    • That's certainly not how shader parameters are supposed to be used, no.

      I think the options here are essentially either a compile option or an extension. The two existing instances of this kind of thing that we have are VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV and VKD3D_SHADER_COMPILE_OPTION_WRITE_TESS_GEOM_POINT_SIZE, so a compile option would be my first choice at this point. If we already know that we're going to need a number of other features like this, we could also make that compile option a set of flags, like we do for e.g. VKD3D_SHADER_COMPILE_OPTION_FORMATTING and VKD3D_SHADER_COMPILE_OPTION_BACKWARD_COMPATIBILITY.

    • Conor McCarthy changed this line in version 4 of the diff

      changed this line in version 4 of the diff

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

    added 31 commits

    • 19257583...803dfc12 - 25 commits from branch wine:master
    • 41711188 - vkd3d-shader/dxil: Introduce an instruction flag to suppress masking of bitwise shift counts.
    • 11da6dce - vkd3d-shader/spirv: Emit an error if 64-bit integers are used.
    • 2a4cd97e - tests/shader-runner: Introduce a 'shader int64' requirement directive.
    • 994bff0b - tests/shader-runner: Add 64-bit arithmetic tests.
    • 11eeaa69 - tests/shader-runner: Add 64-bit bitwise tests.
    • 5eb44296 - vkd3d-shader/spirv: Add a parameter name for int64 capability.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • 64bb5e84 - vkd3d-shader/spirv: Introduce a compiler feature flag for int64 capability.

    Compare with previous version

  • Conor McCarthy added 33 commits

    added 33 commits

    • 64bb5e84...45679a96 - 27 commits from branch wine:master
    • 90a9fb51 - vkd3d-shader/dxil: Introduce an instruction flag to suppress masking of bitwise shift counts.
    • ef7d10ca - vkd3d-shader/spirv: Emit an error if 64-bit integers are used.
    • da76750a - tests/shader-runner: Introduce a 'shader int64' requirement directive.
    • 2e54ea56 - tests/shader-runner: Add 64-bit arithmetic tests.
    • 22a964f7 - tests/shader-runner: Add 64-bit bitwise tests.
    • c1e0abd7 - vkd3d-shader/spirv: Introduce a compiler feature flag for int64 capability.

    Compare with previous version

  • Conor McCarthy added 6 commits

    added 6 commits

    • 1cead9e8 - vkd3d-shader: Introduce an instruction flag to suppress masking of bitwise shift counts.
    • 1480f2dc - vkd3d-shader/spirv: Emit an error if 64-bit integers are used.
    • 292b1165 - tests/shader-runner: Introduce a 'shader int64' requirement directive.
    • acab8258 - tests/shader-runner: Add 64-bit arithmetic tests.
    • beca6043 - tests/shader-runner: Add 64-bit bitwise tests.
    • b2c4ff42 - vkd3d-shader/spirv: Introduce a compiler feature flag for int64 capability.

    Compare with previous version

  • Conor McCarthy changed the description

    changed the description

  • Conor McCarthy added 4 commits

    added 4 commits

    • 36677023 - tests/shader-runner: Introduce an 'int64' requirement directive.
    • fdfffa83 - tests/shader-runner: Add 64-bit arithmetic tests.
    • c20bc1dd - tests/shader-runner: Add 64-bit bitwise tests.
    • e85d5440 - vkd3d-shader/spirv: Introduce a compiler feature flag for int64 capability.

    Compare with previous version

  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit e85d5440
  • 253 261 * \since 1.10
    254 262 */
    255 263 VKD3D_SHADER_COMPILE_OPTION_FRAGMENT_COORDINATE_ORIGIN = 0x00000009,
    264 /** \a value is a member of enum vkd3d_shader_compile_option_feature_flags. */
  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Conor McCarthy added 1 commit

    added 1 commit

    • f2188c19 - vkd3d-shader/spirv: Introduce a compiler feature flag for int64 capability.

    Compare with previous version

    • +    if (flags & VKD3DSGF_ENABLE_INT64)
      +    {
      +        FIXME("Unsupported 64-bit integer ops.\n");
      +        spirv_compiler_error(compiler, VKD3D_SHADER_ERROR_SPV_INT64_UNSUPPORTED,
      +                "Support for 64-bit integers is not implemented.");
      +        flags &= ~VKD3DSGF_ENABLE_INT64;
      +    }

      Are we going to have separate error codes for each missing feature? Or could we e.g. have VKD3D_SHADER_ERROR_SPV_UNSUPPORTED_FEATURE? I suppose we have precedent in VKD3D_SHADER_ERROR_SPV_DESCRIPTOR_IDX_UNSUPPORTED and VKD3D_SHADER_ERROR_SPV_STENCIL_EXPORT_UNSUPPORTED, but in theory we could e.g. decide to rename VKD3D_SHADER_ERROR_SPV_STENCIL_EXPORT_UNSUPPORTED to VKD3D_SHADER_ERROR_SPV_UNSUPPORTED_FEATURE and then use that going forward.

      +[require]
      +shader model >= 5.0
      +
      +[pixel shader todo]
      +uniform double2 a;
      +
      +float4 main() : SV_TARGET
      +{
      +    double x = a.x;
      +    double y = a.y;
      +    return float4(x + y, x - y, x * y, x / y);
      +}

      This is not the first test to use doubles, but the preceding commit makes the issue here perhaps more obvious: doubles aren't universally supported. Direct3D 12 has D3D12_FEATURE_DATA_D3D12_OPTIONS.DoublePrecisionFloatShaderOps, Direct3D 11 has D3D11_FEATURE_DATA_DOUBLES.DoublePrecisionFloatShaderOps, Vulkan has VkPhysicalDeviceFeatures.shaderFloat64, and OpenGL has GL_ARB_gpu_shader_fp64.

    • Are we going to have separate error codes for each missing feature? Or could we e.g. have VKD3D_SHADER_ERROR_SPV_UNSUPPORTED_FEATURE? I suppose we have precedent in VKD3D_SHADER_ERROR_SPV_DESCRIPTOR_IDX_UNSUPPORTED and VKD3D_SHADER_ERROR_SPV_STENCIL_EXPORT_UNSUPPORTED, but in theory we could e.g. decide to rename VKD3D_SHADER_ERROR_SPV_STENCIL_EXPORT_UNSUPPORTED to VKD3D_SHADER_ERROR_SPV_UNSUPPORTED_FEATURE and then use that going forward.

      How much do we care about API stability? Maybe we should retain the old name as an alias?

      This is not the first test to use doubles, but the preceding commit makes the issue here perhaps more obvious: doubles aren't universally supported. Direct3D 12 has D3D12_FEATURE_DATA_D3D12_OPTIONS.DoublePrecisionFloatShaderOps, Direct3D 11 has D3D11_FEATURE_DATA_DOUBLES.DoublePrecisionFloatShaderOps, Vulkan has VkPhysicalDeviceFeatures.shaderFloat64, and OpenGL has GL_ARB_gpu_shader_fp64.

      Right now the code only checks in Vulkan, maybe we should check in the other backends too.

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

    added 6 commits

    • f82c7760 - vkd3d-shader/spirv: Emit an error if 64-bit integers are used.
    • d9ecb2e8 - tests/shader-runner: Introduce an 'int64' requirement directive.
    • e6505b44 - tests/shader-runner: Introduce a 'float64' requirement directive.
    • 63675d3b - tests/shader-runner: Add 64-bit arithmetic tests.
    • f46e657e - tests/shader-runner: Add 64-bit bitwise tests.
    • 54e6a63f - vkd3d-shader/spirv: Introduce a compiler feature flag for int64 capability.

    Compare with previous version

  • Conor McCarthy added 18 commits

    added 18 commits

    • 54e6a63f...9cb43723 - 11 commits from branch wine:master
    • edb5fbdd - vkd3d-shader: Introduce an instruction flag to suppress masking of bitwise shift counts.
    • 7f17f2fb - vkd3d-shader/spirv: Emit an error if 64-bit integers are used.
    • dddc70fc - tests/shader-runner: Introduce an 'int64' requirement directive.
    • dcae8838 - tests/shader-runner: Introduce a 'float64' requirement directive.
    • 3d0b5379 - tests/shader-runner: Add 64-bit arithmetic tests.
    • 97cdb787 - tests/shader-runner: Add 64-bit bitwise tests.
    • fd0f99f9 - vkd3d-shader/spirv: Introduce a compiler feature flag for int64 capability.

    Compare with previous version

  • Are we going to have separate error codes for each missing feature? Or could we e.g. have VKD3D_SHADER_ERROR_SPV_UNSUPPORTED_FEATURE? I suppose we have precedent in VKD3D_SHADER_ERROR_SPV_DESCRIPTOR_IDX_UNSUPPORTED and VKD3D_SHADER_ERROR_SPV_STENCIL_EXPORT_UNSUPPORTED, but in theory we could e.g. decide to rename VKD3D_SHADER_ERROR_SPV_STENCIL_EXPORT_UNSUPPORTED to VKD3D_SHADER_ERROR_SPV_UNSUPPORTED_FEATURE and then use that going forward.

    How much do we care about API stability? Maybe we should retain the old name as an alias?

    The error code is externally visible, but that's not true for the name of the enum element; that's internal.

    This is not the first test to use doubles, but the preceding commit makes the issue here perhaps more obvious: doubles aren't universally supported. Direct3D 12 has D3D12_FEATURE_DATA_D3D12_OPTIONS.DoublePrecisionFloatShaderOps, Direct3D 11 has D3D11_FEATURE_DATA_DOUBLES.DoublePrecisionFloatShaderOps, Vulkan has VkPhysicalDeviceFeatures.shaderFloat64, and OpenGL has GL_ARB_gpu_shader_fp64.

    Right now the code only checks in Vulkan, maybe we should check in the other backends too.

    Yes, we should. I don't mind doing that in a separate MR though, as long as it actually happens.

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard approved this merge request

    approved this merge request

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