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

SPIR-V Unions #15233

Open
Snektron opened this issue Apr 10, 2023 · 4 comments
Open

SPIR-V Unions #15233

Snektron opened this issue Apr 10, 2023 · 4 comments
Labels
arch-spirv Khronos Group SPIR-V
Milestone

Comments

@Snektron
Copy link
Collaborator

Unions are problematic in SPIR-V:

  1. We cannot perform pointer casts in shaders.
  2. We cannot load values using OpSpecConstantOp. This means that any struct which contains a union value needs a creative way to be lowered.
  3. SPIR-V cannot properly set alignment for OpVariable instructions, which means that union variables may not be placed correctly in memory.

The first issue can be resolved by either not allowing unions in shaders, or using a combination of bitcast and field_ptr to convert each struct field separately.

The second issue is currently resolved by ignoring SPIR-V's typed constants, and generating a byte bag. In theory it can be resolved using the same combination of bitcasting, but Im not sure if this is a good solution. Alternatively, in kernels, it can be resolved by generating a specific instantiation of a struct which has all the union members filled in with their active field.

The third issue is a problem in SPIR-V which I've been trying to resolve in the standard, see KhronosGroup/SPIRV-Headers#332.

Related: #15230

@andrewrk andrewrk added the arch-spirv Khronos Group SPIR-V label Apr 10, 2023
@andrewrk andrewrk added this to the 0.12.0 milestone Apr 10, 2023
@andrewrk
Copy link
Member

Tagged unions can be lowered as structs. They have non-well-defined memory layout, which means that the size is allowed to be lowered to anything. And only one field is allowed to be accessed at once; they cannot be used to bitcast between types.

Same thing goes for non-extern untagged unions.

The only thing left is extern unions, which are defined to have the memory layout of the target. When the target is SPIR-V, this is a compile error. No problem.

@Snektron
Copy link
Collaborator Author

One option is to lower unions to a struct that has all fields separate, rather than the same memory location. I think that would work pretty well for shaders, actually. The general advice would then be to just not use unions unless absolutely required, and we can always try to implement some hack in the future.

For Kernels I'd like to support unions properly, and that is already implemented for the most part. In its current form this requires a separate implementation for lowering constants, though.

@ashpil
Copy link

ashpil commented Apr 10, 2023

One option is to lower unions to a struct that has all fields separate, rather than the same memory location. I think that would work pretty well for shaders, actually. The general advice would then be to just not use unions unless absolutely required, and we can always try to implement some hack in the future.

It's a nice workaround but I think it would be quite misleading and surprising. The only time I've ever actually wanted unions in shaders was to store different information in the same bytes (for e.g., a heterogeneous array), and to be able to cast between them, which this would not allow. Plus I think it would be odd for shader and kernel capability to have such different union codegen.

@Snektron
Copy link
Collaborator Author

and to be able to cast between them

I guess this is undefined behavior anyway

Plus I think it would be odd for shader and kernel capability to have such different union codegen.

This would be determined by the OS, but i see your point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-spirv Khronos Group SPIR-V
Projects
None yet
Development

No branches or pull requests

3 participants