Skip to content

Conversation

@spencer-lunarg
Copy link
Contributor

re-attempt at #5539 (which was reverted in #5575)

Going to first fix GLSL to not generate invalid SPIR-V

@spencer-lunarg
Copy link
Contributor Author

Will rebase when KhronosGroup/glslang#3878 in pulled in

@spencer-lunarg spencer-lunarg force-pushed the spencer-lunarg-interface-with-pointers-is-not-fun branch from d9d2bf3 to d2ebdde Compare March 1, 2025 20:29
@spencer-lunarg
Copy link
Contributor Author

@alan-baker sorry I am not able to get on the SPIRV WG calls, but not sure if this was discussed

The only reason this was reverted before is because glslang had a single test using it, but not sure if you have opinions on why this should be allowed (since it has a proper VU in the Vulkan Spec even)

@alan-baker
Copy link
Contributor

I guess why it would be supported would be what's the difference between converting the pointer to 64-bit int passing it between stages and converting back vs just passing the pointer?

@spencer-lunarg
Copy link
Contributor Author

vs just passing the pointer?

The issue is it opens up a large can of worms for how Location works and https://gitlab.khronos.org/vulkan/vulkan/-/issues/3677 we decided that we didn't want to spec it out and just ban things and that is how we got here (also it seems from talking to Baldur, this doesn't actually work everywhere and he had an issue with RenderDoc on it https://gitlab.khronos.org/spirv/SPIR-V/-/issues/779#note_517142)

@alan-baker
Copy link
Contributor

vs just passing the pointer?

The issue is it opens up a large can of worms for how Location works and https://gitlab.khronos.org/vulkan/vulkan/-/issues/3677 we decided that we didn't want to spec it out and just ban things and that is how we got here (also it seems from talking to Baldur, this doesn't actually work everywhere and he had an issue with RenderDoc on it https://gitlab.khronos.org/spirv/SPIR-V/-/issues/779#note_517142)

I'm not sure what the problem for location is? Those pointers are basically 64-bit values. Vulkan can ban it, but it feels like working around implementation bugs. That may be the best choice though.

@spencer-lunarg
Copy link
Contributor Author

I'm not sure what the problem for location is?

So can the Vertex output a Pointer and the Fragment consume a uint64_t/vec2

but it feels like working around implementation bugs.

I think it is less of a bug and just that it was never described in the spec when Buffer Device Address was added and there was one CTS test, so it really lacks coverage and goes back to my "if people want it, they should test it" but I don't see why we should encourage the use of untested features in Vulkan, ones that have been found already to cause issues elsewhere

@alan-baker
Copy link
Contributor

I'm not sure what the problem for location is?

So can the Vertex output a Pointer and the Fragment consume a uint64_t/vec2

Theoretically yes, but...

but it feels like working around implementation bugs.

I think it is less of a bug and just that it was never described in the spec when Buffer Device Address was added and there was one CTS test, so it really lacks coverage and goes back to my "if people want it, they should test it" but I don't see why we should encourage the use of untested features in Vulkan, ones that have been found already to cause issues elsewhere

Yes, pragmatically speaking, banning is probably best.

@alan-baker alan-baker merged commit d3fc6ed into KhronosGroup:main Mar 5, 2025
22 checks passed
@spencer-lunarg spencer-lunarg deleted the spencer-lunarg-interface-with-pointers-is-not-fun branch March 5, 2025 23:00
svenvh added a commit to svenvh/SPIRV-Tools that referenced this pull request Apr 4, 2025
The validation added in KhronosGroup#6000 seems specific to Vulkan environments.
Ensure it only runs for Vulkan environments.

Signed-off-by: Sven van Haastregt <[email protected]>
alan-baker pushed a commit that referenced this pull request Apr 4, 2025
The validation added in #6000 seems specific to Vulkan environments.
Ensure it only runs for Vulkan environments.

Signed-off-by: Sven van Haastregt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants