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 for VK_KHR_zero_initialize_workgroup_memory #2443

Open
squidbus opened this issue Feb 4, 2025 · 2 comments · May be fixed by #2444
Open

Support for VK_KHR_zero_initialize_workgroup_memory #2443

squidbus opened this issue Feb 4, 2025 · 2 comments · May be fixed by #2444

Comments

@squidbus
Copy link
Contributor

squidbus commented Feb 4, 2025

This extension allows initializing workgroup memory in shaders using a null constant. It is also required for Vulkan 1.3 support in MoltenVK.

What seems to be missing currently:

  • Cannot zero-initialize workgroup memory; seems to be unimplemented and blocked by this conditional:

    else if (variable.initializer && !variable_decl_is_remapped_storage(variable, StorageClassWorkgroup))

  • The CTS tests for this use specialization constants for array sizes, which is also unsupported with OpConstantNull:

    if (!constant_type.array_size_literal.back())
    SPIRV_CROSS_THROW("Array size of OpConstantNull must be a literal.");

@dboyan
Copy link
Contributor

dboyan commented Feb 6, 2025

Actually I also happened looking into it recently. Your observation is accurate. I will share my current thinking and attempts here.

Cannot zero-initialize workgroup memory; seems to be unimplemented and blocked by this conditional

For the first one, I would say to keep the current logic for non-workgroup memory. But for workgroup (i.e., shared in GLSL, threadgroup in MSL) memory with initializer, we add a simplified logic as I believe workgroup memory can only be initialized with OpConstantNull (Reference: Validation logic in SPIRV-Tools as of now).

Now the question becomes how to initialize the memory in different shading languages. In GLSL this is simple, we can just use GL_EXT_null_initializer and initialize everything to { }.

In MSL, this becomes trickier. I just attempted the cheapest way out, namely trying to use initializer on threadgroup variables (as MSL "follows" C++14 it should support this, although I have no idea how they map it into implementation). I initialize array or composite types with { } and other types with their usual initializers. MSL compiler seemed happy with my logic and surprisingly it passed a bunch of CTS tests. However, there are 19 failing tests (iirc). I haven't got the time to look into the details of those passing and failing tests so I'm not sure if the correctness of this approach can be fixed by adding a barrier or if this approach is fundamentally flawed.

If the above approach doesn't work, the correct approach should be generating a code snippet after threadgroup variable definition and let the threads cooperatively initialize the memory, and then add a threadgroup barrier. Just like the lowering logic in NIR and the example snippet from Dawn.

I've no idea about HLSL, or if we want to support it there at all.

specialization constants for array sizes, which is also unsupported with OpConstantNull

Allowing specialization constant here will affect the code that follows, which (I think) is mainly related to the initializer generation. My current logic (hack) is that if the array size is not literal, assume that it has one element to initialize. Apparently we cannot use the default value of the specialization constant as it can be smaller after specialization, and the compiler won't complain if we have less elements than there actually are in the initializer list. I'm not very sure if I am missing something.

@dboyan
Copy link
Contributor

dboyan commented Feb 6, 2025

Also, using specialization constants for array sizes is problematic in MoltenVK (KhronosGroup/MoltenVK#2423). A proper fix requires the aid of SPIRV-Cross, and I have #2442 here and KhronosGroup/MoltenVK#2434 for MoltenVK. Unfortunately I haven't got a review on either side yet.

For my own CTS runs I have applied my fix for specialization constants locally.

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 a pull request may close this issue.

2 participants