vkd3d-shader: Prepare for 64-bit integer support.
Merge request reports
Activity
added 1 commit
- 19257583 - vkd3d-shader/spirv: Add a parameter name for int64 capability.
mentioned in merge request !490 (merged)
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.
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.)
changed this line in version 5 of the diff
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); 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 ofEXT_DESCRIPTOR_INDEXING
andEXT_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 forEXT_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.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.
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?
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.
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.
changed this line in version 4 of the diff
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.
Toggle commit list-
19257583...803dfc12 - 25 commits from branch
added 1 commit
- 64bb5e84 - vkd3d-shader/spirv: Introduce a compiler feature flag for int64 capability.
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.
Toggle commit list-
64bb5e84...45679a96 - 27 commits from branch
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.
Toggle commit listadded 4 commits
Toggle commit list253 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. */ changed this line in version 8 of the diff
added 1 commit
- f2188c19 - vkd3d-shader/spirv: Introduce a compiler feature flag for int64 capability.
+ 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.
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.
Toggle commit listadded 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.
Toggle commit list-
54e6a63f...9cb43723 - 11 commits from branch
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.