Skip to content
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

Support SPIR-V 1.6 #3943

Open
nanokatze opened this issue Apr 13, 2024 · 4 comments
Open

Support SPIR-V 1.6 #3943

nanokatze opened this issue Apr 13, 2024 · 4 comments

Comments

@nanokatze
Copy link

nanokatze commented Apr 13, 2024

A module specifying version 1.6 impliesVK_PIPELINE_SHADER_STAGE_CREATE_ALLOW_VARYING_SUBGROUP_SIZE_BIT, which makes gl_SubgroupSize/WaveGetLaneCount behave on implementations with several subgroup sizes in a way that better aligns with developer expectations. Developers using shaders compiled with current slang have to specify that flag explicitly in their host code.

1.6 also promoted DemoteToHelperInvocation to core which I suspect is what slang would want to use when lowering discard.

Of the non-user-visible changes, SPIR-V 1.6 disallows having then and else of BranchConditional be the same block. I stumbled upon a comment

// TODO(JS):
// Was previously set to SpvVersion, but that doesn't work since we
// upgraded to SPIR-V headers 1.6. (It would lead to validation errors during vk tests)
// For now mark as version 1.5.0
static const uint32_t spvVersion1_5_0 = 0x00010500;
m_words.add(spvVersion1_5_0);

which maybe refers to that occurring? I patched slang to emit 1.6 but wasn't able to trigger any validation diagnostics with my shaders which are admittedly relatively simple.

Also, while to this date no features in Vulkan are banned from SPIR-V 1.5, at most Kill and WorkgroupSize are deprecated (but not removed), I suspect that at some point that might become the case.

@csyonghe
Copy link
Collaborator

We may need to move to spirv 1.6 as a user opt-in option instead of doing it wholesale, so some of those workaround logic still need to exist until we can confirm no one is still relying on 1.5.

@nanokatze
Copy link
Author

nanokatze commented Apr 13, 2024

Right, I wasn't sure whether it was intended for slang to always target the latest spir-v version, that comment made me think as if it is. Could you elaborate on what do you mean with "workaround logic"? 1.6 doesn't forbid anything you'd do when targeting 1.5 except the thing with BranchConditional.

If slang already satisfies that requirement (never generates BranchConditional where true and else blocks are the same) it seems like it's just a matter of plumbing glsl_spirv_1_N into SPIR-V emitter which I'd be willing to work on, but I'm kinda concerned about that comment in the snippet in the OP and it's not very clear as to which tests it's referring to. I guess I should just try running all slang/tests and see what blows up.

@csyonghe
Copy link
Collaborator

That comment is obsolete and you can disregard it. We are currently targeting 1.5 intentionally. We don't aim to always support latest version defined in the header because we need to support users who can't target a newer spirv model.

We should allow dynamic selection of target spirv version, in that if the user calls a spirv 1.6 function or specified the spirv 1.6 target profile explicitly, then we generate spirv according to the 1.6 spec.

@csyonghe
Copy link
Collaborator

And we definitely welcome/appreciate your contribution! Let me know if you need any input or help from us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants