spirv: miscellaneous stuff #22889

alichraghi · 2025-02-14T17:01:10Z

lib/std/Target.zig

alexrp · 2025-02-14T17:09:54Z

lib/std/Target/spirv.zig

-    SPV_AMD_shader_fragment_mask,
-    SPV_AMD_gpu_shader_int16,
+    v1_6,
+    SPV_AMDX_shader_enqueue,


It's totally fine if you don't feel like doing this, but bonus points for making the script fix these names, i.e. amdx_shader_enqueue.

yeah i was actually going to ask is it okay to add helper functions like featureToExtension to Target/spirv.zig

Can you elaborate? What would that function do?

returns the actual extension name of a feature. currently we basically do @tagName(feature) but that would no longer work if we turn features name to snake_case

never mind. im removing all those unknown features. if someone wants to emit a random capability/extension they can do it through inline assembly. all cpu features must be recognizable by the compiler

To be clear, I'm not actually opposed to having all these features if there's at least a conceivable world where some compiler-adjacent tooling might be interested in representing/querying them - even if that's not the Zig compiler specifically. For example, I consider it a design goal of std.Target to be usable for tools like dis/assemblers, decompilers, emulators, compilers for other languages, etc...

Good point re: mapping Ziggified names to SPIR-V names; I hadn't thought about that aspect. Ordinarily, we don't add helper functions of this nature in std.Target.<arch>, but that's mostly an artifact of the feature bits not necessarily corresponding to some official set of features. Since the data is readily available when generating the feature list, I don't see any harm in adding a method along the lines of extensionName(feature: Feature) [:0]const u8 for SPIR-V. Obviously only relevant if we decide to go back to that approach rather than the new one you proposed here.

Definitely interested in @Snektron's perspective on all this.

`OpCapability` and `OpExtension` now can also be emitted from inline assembly

Snektron

Looks pretty good imo. I'm not completely convinced yet about the target features, but I guess this is better than what it was.

Snektron · 2025-02-14T22:20:58Z

lib/std/gpu.zig

+pub fn binding(comptime ptr: anytype, comptime descriptor_set: u32, comptime bind: u32) void {
+    asm volatile ("OpDecorate %ptr DescriptorSet " ++ &[_]u8{'0' + descriptor_set} ++
+            "OpDecorate %ptr Binding " ++ &[_]u8{'0' + bind}
        :
        : [ptr] "" (ptr),
    );


I think that this should work

Suggested change

pub fn binding(comptime ptr: anytype, comptime descriptor_set: u32, comptime bind: u32) void {

asm volatile ("OpDecorate %ptr DescriptorSet " ++ &[_]u8{'0' + descriptor_set} ++

"OpDecorate %ptr Binding " ++ &[_]u8{'0' + bind}

:

: [ptr] "" (ptr),

);

pub fn binding(comptime ptr: anytype, comptime descriptor_set: u32, comptime bind: u32) void {

asm volatile (

\\OpDecorate %ptr DescriptorSet $set

\\OpDecorate %ptr Binding $bind

:

: [ptr] "" (ptr),

[set] "c" (set),

[bind] "c" (bind),

);

this $constant syntax was added a while ago I think

Snektron · 2025-02-14T22:22:09Z

lib/std/Target/spirv.zig

-    ShaderStereoViewNV,
-    PerViewAttributesNV,
-    FragmentFullyCoveredEXT,
-    MeshShadingNV,
-    ImageFootprintNV,
-    FragmentBarycentricNV,
-    ComputeDerivativeGroupQuadsNV,
-    FragmentDensityEXT,
-    ShadingRateNV,
-    GroupNonUniformPartitionedNV,
-    ShaderNonUniform,
-    ShaderNonUniformEXT,
-    RuntimeDescriptorArray,
-    RuntimeDescriptorArrayEXT,
-    InputAttachmentArrayDynamicIndexing,
-    InputAttachmentArrayDynamicIndexingEXT,
-    UniformTexelBufferArrayDynamicIndexing,
-    UniformTexelBufferArrayDynamicIndexingEXT,
-    StorageTexelBufferArrayDynamicIndexing,
-    StorageTexelBufferArrayDynamicIndexingEXT,
-    UniformBufferArrayNonUniformIndexing,
-    UniformBufferArrayNonUniformIndexingEXT,
-    SampledImageArrayNonUniformIndexing,
-    SampledImageArrayNonUniformIndexingEXT,
-    StorageBufferArrayNonUniformIndexing,
-    StorageBufferArrayNonUniformIndexingEXT,
-    StorageImageArrayNonUniformIndexing,
-    StorageImageArrayNonUniformIndexingEXT,
-    InputAttachmentArrayNonUniformIndexing,
-    InputAttachmentArrayNonUniformIndexingEXT,
-    UniformTexelBufferArrayNonUniformIndexing,
-    UniformTexelBufferArrayNonUniformIndexingEXT,
-    StorageTexelBufferArrayNonUniformIndexing,
-    StorageTexelBufferArrayNonUniformIndexingEXT,
-    RayTracingNV,
-    VulkanMemoryModel,
-    VulkanMemoryModelKHR,
-    VulkanMemoryModelDeviceScope,
-    VulkanMemoryModelDeviceScopeKHR,
-    PhysicalStorageBufferAddresses,
-    PhysicalStorageBufferAddressesEXT,
-    ComputeDerivativeGroupLinearNV,
-    RayTracingProvisionalKHR,
-    CooperativeMatrixNV,
-    FragmentShaderSampleInterlockEXT,
-    FragmentShaderShadingRateInterlockEXT,
-    ShaderSMBuiltinsNV,
-    FragmentShaderPixelInterlockEXT,
-    DemoteToHelperInvocationEXT,
-    SubgroupShuffleINTEL,
-    SubgroupBufferBlockIOINTEL,
-    SubgroupImageBlockIOINTEL,
-    SubgroupImageMediaBlockIOINTEL,
-    RoundToInfinityINTEL,
-    FloatingPointModeINTEL,
-    IntegerFunctions2INTEL,
-    FunctionPointersINTEL,
-    IndirectReferencesINTEL,
-    AsmINTEL,
-    AtomicFloat32MinMaxEXT,
-    AtomicFloat64MinMaxEXT,
-    AtomicFloat16MinMaxEXT,
-    VectorComputeINTEL,
-    VectorAnyINTEL,
-    ExpectAssumeKHR,
-    SubgroupAvcMotionEstimationINTEL,
-    SubgroupAvcMotionEstimationIntraINTEL,
-    SubgroupAvcMotionEstimationChromaINTEL,
-    VariableLengthArrayINTEL,
-    FunctionFloatControlINTEL,
-    FPGAMemoryAttributesINTEL,
-    FPFastMathModeINTEL,
-    ArbitraryPrecisionIntegersINTEL,
-    UnstructuredLoopControlsINTEL,
-    FPGALoopControlsINTEL,
-    KernelAttributesINTEL,
-    FPGAKernelAttributesINTEL,
-    FPGAMemoryAccessesINTEL,
-    FPGAClusterAttributesINTEL,
-    LoopFuseINTEL,
-    FPGABufferLocationINTEL,
-    USMStorageClassesINTEL,
-    IOPipesINTEL,
-    BlockingPipesINTEL,
-    FPGARegINTEL,
-    AtomicFloat32AddEXT,
-    AtomicFloat64AddEXT,
-    LongConstantCompositeINTEL,
+    v1_6,
+    int8,
+    int16,
+    int64,
+    float16,
+    float64,
+    addresses,
+    matrix,
+    kernel,
+    generic_pointer,
+    vector16,
+    shader,


Are these the same features as LLVM?

LLVM doesn't define any features (yet?).
https://github.com/llvm/llvm-project/blob/5c93eb56dc9bc0c0210483cdd5d31e6b6580454f/llvm/lib/Target/SPIRV/SPIRV.td

Snektron · 2025-02-14T22:25:35Z

tools/update_cpu_features.zig

-    // TODO: merge tools/update_spirv_features.zig into this script
-    //.{
-    //    .zig_name = "spirv",
-    //    .llvm_name = "SPIRV",
-    //    .td_name = "SPIRV.td",
-    //},


Maybe we need something equivalent to this again with LLVM 20? Since SPIR-V is enabled by default in LLVM 20?

Do we actually want to enable using LLVM's SPIR-V backend with Zig? I'd brought that up a while back (I think around the time of the LLVM 19 upgrade?) and there seemed to be some resistance to the idea.

Right now, we explicitly prevent it:

zig/src/codegen/llvm.zig

Lines 13019 to 13023 in 8a3aeba

// We don't currently support using these backends.

.spirv,

.spirv32,

.spirv64,

=> {},

We can't do that yet because they define no features

Snektron · 2025-02-14T22:29:31Z

src/codegen/spirv/Module.zig

+        if (std.Target.spirv.featureSetHas(target.cpu.features, .v1_0)) break :blk 0;
+        // TODO: always assume a version set?


I guess we can always assume to have SPIR-V 1.0

Snektron · 2025-02-14T22:47:59Z

src/codegen/spirv.zig

-        const key = .{ child_ty.toIntern(), storage_class, child_repr };
-        const entry = try self.ptr_types.getOrPut(self.gpa, key);
-        if (entry.found_existing) {
-            const fwd_id = entry.value_ptr.ty_id;
-            if (!entry.value_ptr.fwd_emitted) {
-                try self.spv.sections.types_globals_constants.emit(self.spv.gpa, .OpTypeForwardPointer, .{
-                    .pointer_type = fwd_id,
-                    .storage_class = storage_class,
-                });
-                entry.value_ptr.fwd_emitted = true;
-            }
-            return fwd_id;
-        }
-
-        const result_id = self.spv.allocId();
-        entry.value_ptr.* = .{
-            .ty_id = result_id,
-            .fwd_emitted = false,
-        };
-
+    fn ptrType(self: *NavGen, child_ty: Type, storage_class: StorageClass, child_repr: Repr) !IdRef {
        const child_ty_id = try self.resolveType(child_ty, child_repr);
-
-        try self.spv.sections.types_globals_constants.emit(self.spv.gpa, .OpTypePointer, .{
-            .id_result = result_id,
-            .storage_class = storage_class,
-            .type = child_ty_id,
-        });
-
+        const result_id = try self.spv.ptrType(child_ty_id, storage_class);
+        if (storage_class == .Uniform or storage_class == .PushConstant) try self.spv.decorate(child_ty_id, .Block);
+        try self.spv.decorate(result_id, .{ .ArrayStride = .{ .array_stride = @intCast(child_ty.abiSize(self.pt.zcu)) } });
        return result_id;
    }


This is not the right way to handle things because it doesn't correctly handle recursive pointers. The call to self.resolveType() would enter an infinite loop, thats why the ptr_types map is handled especially. For example, this kernel fails:

const Node = struct { node: *Node }; export fn oef() callconv(.spirv_kernel) void { var a: Node = undefined; _ = &a; }

~/programming/zig/build/spirv-zig/bin/zig build-obj ./test.zig -fno-compiler-rt -target spirv64-ope ncl-gnu -mcpu generic+int64+int16+int8+float64+float16+generic_pointer+v1_6+kernel -fno-llvm -freference-trace fish: Job 1, '~/programming/zig/build/spirv-z…' terminated by signal SIGSEGV (Address boundary error)

In the future we might be able to remove this ugly piece of code by relying on SPV_KHR_untyped_pointers. Some projects are already adding support for this, so hopefully we will be able to use this in the not too far away future.

alichraghi added 3 commits February 13, 2025 16:14

std.gpu: stop using comptimePrint

a2b8d91

spirv: cache more types & merge constructX functions

65a8c90

target: update spirv features

f5a1da1

alichraghi force-pushed the ali_spv branch from 9dbf956 to 4fc203e Compare February 14, 2025 17:04

alexrp reviewed Feb 14, 2025

View reviewed changes

lib/std/Target.zig Outdated Show resolved Hide resolved

alexrp reviewed Feb 14, 2025

View reviewed changes

spirv: respect cpu features

e047830

alichraghi force-pushed the ali_spv branch 2 times, most recently from a3f7b49 to b7782ce Compare February 14, 2025 20:39

spirv: ziggify and remove unknown spirv features

051feba

`OpCapability` and `OpExtension` now can also be emitted from inline assembly

alichraghi force-pushed the ali_spv branch from b7782ce to 051feba Compare February 14, 2025 21:27

Snektron reviewed Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spirv: miscellaneous stuff #22889

spirv: miscellaneous stuff #22889

alichraghi commented Feb 14, 2025

alexrp Feb 14, 2025

alichraghi Feb 14, 2025

alexrp Feb 14, 2025

alichraghi Feb 14, 2025

alichraghi Feb 14, 2025 •

edited

Loading

alexrp Feb 15, 2025

Snektron left a comment

Snektron Feb 14, 2025

Snektron Feb 14, 2025

alichraghi Feb 15, 2025

Snektron Feb 14, 2025

alexrp Feb 15, 2025

alichraghi Feb 15, 2025

Snektron Feb 14, 2025

Snektron Feb 14, 2025

Snektron Feb 14, 2025

	// We don't currently support using these backends.
	.spirv,
	.spirv32,
	.spirv64,
	=> {},

		if (std.Target.spirv.featureSetHas(target.cpu.features, .v1_0)) break :blk 0;
		// TODO: always assume a version set?

spirv: miscellaneous stuff #22889

Are you sure you want to change the base?

spirv: miscellaneous stuff #22889

Conversation

alichraghi commented Feb 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alichraghi Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snektron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alichraghi Feb 14, 2025 •

edited

Loading