ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
32.38k stars 2.36k forks source link

SPIR-V Unions #15233

Open Snektron opened 1 year ago

Snektron commented 1 year ago

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 https://github.com/KhronosGroup/SPIRV-Headers/issues/332.

Related: #15230

andrewrk commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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