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

Allocating local mem in device functions is not explictly forbidden by the SPIR-V OpenCL Env #1221

Open
pjaaskel opened this issue Aug 8, 2024 · 5 comments

Comments

@pjaaskel
Copy link
Contributor

pjaaskel commented Aug 8, 2024

Currently it's possible to create a SPIR-V that allocates local mem in non-kernel functions although this is explicitly forbidden in OpenCL C spec for obvious implementation challenges. It is even accepted by various drivers (which likely just inline everything anyhow, effectively making the allocation happen inside the kernel).

Shall we add a check to the SPIR-V OpenCL Env for this case?

@pjaaskel pjaaskel changed the title Allocating local mem in device functions is not explictly forbid by the SPIR-V OpenCL Env Allocating local mem in device functions is not explictly forbidden by the SPIR-V OpenCL Env Aug 8, 2024
@bashbaug
Copy link
Contributor

bashbaug commented Aug 8, 2024

The easiest thing to do here would be to add text similar to that in the OpenCL C spec, something like:
https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_C.html#_usage_for_declaration_scopes_and_variable_types

Variables declared in the outermost compound statement inside the body of the kernel function can be qualified by the local or constant address spaces.

However, there are two real-world problems with this proposed resolution:

  1. We added a pass in the SPIR-V LLVM Translator a while back to handle the SPIR-V restriction that a SPIR-V function cannot call a SPIR-V kernel ("entry point"): add an entry point wrapper around functions (llvm pass) SPIRV-LLVM-Translator#1149. The "entry point wrapper" that gets inserted will therefore call a function with the contents of the kernel and hence the local memory declaration, meaning the local memory declaration will no longer be "inside the body of the kernel function".

  2. Additionally, Clang currently takes a local memory declaration within a kernel (or elsewhere!) and turns it into global scope declaration, which is faithfully translated to a global scope declaration by the SPIR-V LLVM Translator: https://godbolt.org/z/eE4Yd8sr4. This means with typical SPIR-V toolchains I believe a SPIR-V consumer will only see global scope local memory allocations and will never see a local memory allocation within a function.

Should we document the global scope requirement in the OpenCL SPIR-V environment spec, since this is what the usual SPIR-V generation toolchains is generating?

@pjaaskel
Copy link
Contributor Author

pjaaskel commented Aug 8, 2024

Should we document the global scope requirement in the OpenCL SPIR-V environment spec, since this is what the usual SPIR-V generation toolchains is generating?

I think yes, and perhaps explicitly allow defining locals in device functions even for OpenCL C since it practically likely ("accidentally") works for every driver anyhow.

@bashbaug
Copy link
Contributor

bashbaug commented Aug 8, 2024

Actually, maybe we don't need to do anything. The SPIR-V spec already says:
https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_logical_layout_of_a_module

All OpVariable instructions in a function must have a Storage Class of Function.

This means that the transformation Clang is doing is the only valid way to use local memory in SPIR-V.

@pjaaskel
Copy link
Contributor Author

pjaaskel commented Aug 9, 2024

OK, so the main question that remains is that should we lift the OpenCL C restriction as well assuming all/most are using the Clang's implementation for locals.

@bashbaug
Copy link
Contributor

Discussed in the August 20th teleconference. There is some interest in lifting the OpenCL C restriction (probably as an extension?), which should be a pretty light lift for most implementors that are using Clang. I've moved this back to "To do" until somebody is able to pick this up, since there is (currently) nothing more to discuss in the working group.

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

No branches or pull requests

2 participants