-
Notifications
You must be signed in to change notification settings - Fork 718
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPIR-V] Compiling with doubles may also implicitly enable Int64 capability (which may or may not be supported) #7038
Comments
You will find missing F64 before you find missing I64. vulkan 1.3 onwards BDA is encouraged, and DXC doesn't even emit the uvec2 variant of BDA, so one might say that if one targets VK 1.3 int64 is very strongly implied. |
Hi! Shorter repro: // RUN: %dxc -E main -T cs_6_5 -spirv %s | FileCheck %s
ByteAddressBuffer input;
RWByteAddressBuffer output;
[numthreads(1, 1, 1)]
void main() {
output.Store<double>(0, input.Load<double>(0));
} This indeed emits the Int64 capability, but that's because of the way ByteAddressBuffer are loaded/stored to: ByteAddressBuffer content are supposed to be unsigned integers, and then the data can be interpreted as something else. This means to handle a 32-bit float, we need to:
To handle 64bit floats, we need to:
SPIR-V doesn't allow shift operations on floats, hence using an integer is a requirement to recompose a double from 2 integers. For this reason, we have to declare the Int64 capability when reading/writting a Float64 from the ByteAddressBuffer. The optimization we COULD possible have is to prevent the cast to a float when not required. Ex: if the loaded float/double is not modified before being stored back into another buffer. Now, do we want to implement this now? I don't think so. |
Fair enough, will deal with it the normal way in the compiler then. F64 will hard require I64. |
From Core3 docs, seems like you only target Vulkan1.2+ . |
I think sometimes they add an extension but the capability itself is disabled. But I'm going to check if there's any device that might have a problem with this. The vulkan database is probably back online again. |
Right, there is something weird, looks like you need to support/understand the extension, but you can say you don't support the feature..
|
I think it's fine honestly, it seems like there are only a few android devices that support F64 and they all support I64. Shouldn't cause any problems I think |
Out of curiosity, is there any overhead to the casts that are being done? Is it possible that there will be a native load replacement rather than uint -> bitcast in the future? Or is that just a spirv limitation |
No idea, it will all depend on the target ISA. |
May I treat to you some Nabla-isms? You can use OpBitcast between a SPIR-V spec allows bitcasts between native SPIR-V (scalars, vectors of scalars, etc.) as long as the two operands are the same size. It also allows for casts between uint64 and PhysicalStoragePointer but thats with the BDA capability. P.S. there's a slight pita with SPIR-V OpBitcast, it doesn't support identity |
Oh completely forgot that you can mismatch the element count as long as bitcount is fine! (modulus the rule around component number being a multiple of the other). I quickly looked at the code, and looks like the classic |
You could probably improve the SPIR-V codegen for With this SPIR-V bitcast, it means that you can load every final PoD member into a vector/array of the appropriate number of |
Goated, thanks guys <3 |
When loading a Float64 from a raw buffer, we used an Int64, which required an additional capability, even if the code wasn't using any Int64. In practice, it seems most devices supporting Float64 do also support Int64, but this it doesn't have to. By changing the codegen a bit, we can avoid the Int64 value. Fixes microsoft#7038 Signed-off-by: Nathan Gauër <[email protected]>
When loading a Float64 from a raw buffer, we used an Int64, which required an additional capability, even if the code wasn't using any Int64. In practice, it seems most devices supporting Float64 do also support Int64, but this it doesn't have to. By changing the codegen a bit, we can avoid the Int64 value. Tested the word-order using a vulkan compute shader, and checking the returned value on the API side. ```hlsl double tmp = buffer.Load<double>(0); if (tmp == 12.0) buffer.Store<double>(0, 13.0); ``` Fixes microsoft#7038 Signed-off-by: Nathan Gauër <[email protected]>
Description
When loading and adding a 64-bit float from a BAB it produces 64-bit int instructions as well, even though not used in the source.
Steps to Reproduce
Turns into roughly:
E.g.
%66 = OpUConvert %ulong %63 ; 0x00000634
is emitted before working on the double, requiring the capability.Actual Behavior
Just like DXIL don't start enabling capabilities that aren't used by the shader (DXIL only enables the 64-bit float extension rather than both).
From the D3D12 database I can't find a device that doesn't have both I64 and F64, but the vulkan database is offline so I'm not sure if any such device exists for Vulkan (for example some mobile device).
Environment
The text was updated successfully, but these errors were encountered: