search

KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
2.9k stars 816 forks source link

NV_shader_atomic_fp16_vector #3519

Closed jeffbolznv closed 4 months ago

jeffbolznv commented 4 months ago

This implements the old OpenGL/GLSL extension https://registry.khronos.org/OpenGL/extensions/NV/NV_shader_atomic_fp16_vector.txt using the new SPIR-V extension https://htmlpreview.github.io/?https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/NV/SPV_NV_shader_atomic_fp16_vector.html

AnyOldName3 commented 4 months ago

First things first, I don't know if this is how things are supposed to work, so I apologise for the noise if I'm unnecessarily raising red flags.

After this PR was merged, the new test fails unless SPIRV-Tools and SPIRV-Headers are updated to the new commit in known_good.json. That's fine on its own (although it briefly made me think I'd broken something on my end), but I imagine that if ALLOW_EXTERNAL_SPIRV_TOOLS is set and the system-installed library is used, it will probably be an older commit and the test will fail there, too. I just wanted to make sure no one had forgotten to add some conditional compilation if a feature check failed.

If instead it's expected that some tests fail with commits other than the one in known_good.json, it might be a good idea to document that, e.g. in the testing part of the readme and the docstring for the ALLOW_EXTERNAL_SPIRV_TOOLS CMake option.

arcady-lunarg commented 4 months ago

The only supported configuration is with the versions of headers/tools in known_good.json. If you're using your own version of SPIRV-Tools, you're on your own. That's why we didn't even have the ALLOW_EXTERNAL_SPIRV_TOOLS. @AnyOldName3 if you think the documentation around this option could be improved, patches are certainly welcome.