From 08ad7afc1e90c68409e9dc869d9dc1dceb3e8564 Mon Sep 17 00:00:00 2001 From: Robin Voetter Date: Thu, 31 Oct 2024 20:24:37 +0100 Subject: [PATCH 01/10] spirv: forbid pointer arithmetic --- src/Sema.zig | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/Sema.zig b/src/Sema.zig index 65805b6a67..771f4a0540 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -17605,6 +17605,24 @@ fn analyzePtrArithmetic( } else break :rs ptr_src; }; + const target = zcu.getTarget(); + if (target_util.arePointersLogical(target, ptr_info.flags.address_space)) { + return sema.failWithOwnedErrorMsg(block, msg: { + const msg = try sema.errMsg(op_src, "illegal pointer arithmetic on pointer of type '{}'", .{ptr_ty.fmt(pt)}); + errdefer msg.destroy(sema.gpa); + + const backend = target_util.zigBackend(target, zcu.comp.config.use_llvm); + try sema.errNote(op_src, msg, "arithmetic cannot be performed on pointers with address space '{s}' on target {s}-{s} by compiler backend {s}", .{ + @tagName(ptr_info.flags.address_space), + target.cpu.arch.genericName(), + @tagName(target.os.tag), + @tagName(backend), + }); + + break :msg msg; + }); + } + try sema.requireRuntimeBlock(block, op_src, runtime_src); return block.addInst(.{ .tag = air_tag, From 4fbc100959c05ba10e4bfeeca9c7b5cd8216cc57 Mon Sep 17 00:00:00 2001 From: Robin Voetter Date: Sat, 2 Nov 2024 12:34:02 +0100 Subject: [PATCH 02/10] spirv: properly resolve type inputs in assembly For now the frontend still allows type inputs in assembly. We might as well resolve them properly in the SPIR-V backend. --- src/codegen/spirv.zig | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/codegen/spirv.zig b/src/codegen/spirv.zig index 34497d8de8..2bb4f1d3e0 100644 --- a/src/codegen/spirv.zig +++ b/src/codegen/spirv.zig @@ -1662,7 +1662,7 @@ const NavGen = struct { else => unreachable, } - // Guaranteed by callConvSupportsVarArgs, there are nog SPIR-V CCs which support + // Guaranteed by callConvSupportsVarArgs, there are no SPIR-V CCs which support // varargs. assert(!fn_info.is_var_args); @@ -6580,8 +6580,16 @@ const NavGen = struct { // for the string, we still use the next u32 for the null terminator. input_extra_i += (constraint.len + name.len + (2 + 3)) / 4; - const value = try self.resolve(input); - try as.value_map.put(as.gpa, name, .{ .value = value }); + if (self.typeOf(input).zigTypeTag(zcu) == .type) { + // This assembly input is a type instead of a value. + // That's fine for now, just make sure to resolve it as such. + const val = (try self.air.value(input, self.pt)).?; + const ty_id = try self.resolveType(val.toType(), .direct); + try as.value_map.put(as.gpa, name, .{ .ty = ty_id }); + } else { + const val_id = try self.resolve(input); + try as.value_map.put(as.gpa, name, .{ .value = val_id }); + } } as.assemble() catch |err| switch (err) { From 7682ced08ea44d7535031d2c6075dc7ca28722a6 Mon Sep 17 00:00:00 2001 From: Robin Voetter Date: Sat, 2 Nov 2024 15:22:48 +0100 Subject: [PATCH 03/10] spirv: track global OpVariables properly in assembler Also cleans up the assembler a bit in general. --- src/codegen/spirv.zig | 51 ++++++++++++--------------------- src/codegen/spirv/Assembler.zig | 35 +++++++++++++++------- 2 files changed, 43 insertions(+), 43 deletions(-) diff --git a/src/codegen/spirv.zig b/src/codegen/spirv.zig index 2bb4f1d3e0..02e0d75d18 100644 --- a/src/codegen/spirv.zig +++ b/src/codegen/spirv.zig @@ -6529,6 +6529,13 @@ const NavGen = struct { return self.todo("implement inline asm with more than 1 output", .{}); } + var as = SpvAssembler{ + .gpa = self.gpa, + .spv = self.spv, + .func = &self.func, + }; + defer as.deinit(); + var output_extra_i = extra_i; for (outputs) |output| { if (output != .none) { @@ -6541,7 +6548,6 @@ const NavGen = struct { // TODO: Record output and use it somewhere. } - var input_extra_i = extra_i; for (inputs) |input| { const extra_bytes = std.mem.sliceAsBytes(self.air.extra[extra_i..]); const constraint = std.mem.sliceTo(extra_bytes, 0); @@ -6549,36 +6555,6 @@ const NavGen = struct { // This equation accounts for the fact that even if we have exactly 4 bytes // for the string, we still use the next u32 for the null terminator. extra_i += (constraint.len + name.len + (2 + 3)) / 4; - // TODO: Record input and use it somewhere. - _ = input; - } - - { - var clobber_i: u32 = 0; - while (clobber_i < clobbers_len) : (clobber_i += 1) { - const clobber = std.mem.sliceTo(std.mem.sliceAsBytes(self.air.extra[extra_i..]), 0); - extra_i += clobber.len / 4 + 1; - // TODO: Record clobber and use it somewhere. - } - } - - const asm_source = std.mem.sliceAsBytes(self.air.extra[extra_i..])[0..extra.data.source_len]; - - var as = SpvAssembler{ - .gpa = self.gpa, - .src = asm_source, - .spv = self.spv, - .func = &self.func, - }; - defer as.deinit(); - - for (inputs) |input| { - const extra_bytes = std.mem.sliceAsBytes(self.air.extra[input_extra_i..]); - const constraint = std.mem.sliceTo(extra_bytes, 0); - const name = std.mem.sliceTo(extra_bytes[constraint.len + 1 ..], 0); - // This equation accounts for the fact that even if we have exactly 4 bytes - // for the string, we still use the next u32 for the null terminator. - input_extra_i += (constraint.len + name.len + (2 + 3)) / 4; if (self.typeOf(input).zigTypeTag(zcu) == .type) { // This assembly input is a type instead of a value. @@ -6592,7 +6568,18 @@ const NavGen = struct { } } - as.assemble() catch |err| switch (err) { + { + var clobber_i: u32 = 0; + while (clobber_i < clobbers_len) : (clobber_i += 1) { + const clobber = std.mem.sliceTo(std.mem.sliceAsBytes(self.air.extra[extra_i..]), 0); + extra_i += clobber.len / 4 + 1; + // TODO: Record clobber and use it somewhere. + } + } + + const asm_source = std.mem.sliceAsBytes(self.air.extra[extra_i..])[0..extra.data.source_len]; + + as.assemble(asm_source) catch |err| switch (err) { error.AssembleFail => { // TODO: For now the compiler only supports a single error message per decl, // so to translate the possible multiple errors from the assembler, emit diff --git a/src/codegen/spirv/Assembler.zig b/src/codegen/spirv/Assembler.zig index 9e39f2ed09..8fcbad7189 100644 --- a/src/codegen/spirv/Assembler.zig +++ b/src/codegen/spirv/Assembler.zig @@ -151,7 +151,8 @@ gpa: Allocator, errors: std.ArrayListUnmanaged(ErrorMsg) = .empty, /// The source code that is being assembled. -src: []const u8, +/// This is set when calling `assemble()`. +src: []const u8 = undefined, /// The module that this assembly is associated to. /// Instructions like OpType*, OpDecorate, etc are emitted into this module. @@ -211,7 +212,10 @@ pub fn deinit(self: *Assembler) void { self.instruction_map.deinit(self.gpa); } -pub fn assemble(self: *Assembler) Error!void { +pub fn assemble(self: *Assembler, src: []const u8) Error!void { + self.src = src; + self.errors.clearRetainingCapacity(); + // Populate the opcode map if it isn't already if (self.instruction_map.count() == 0) { const instructions = spec.InstructionSet.core.instructions(); @@ -369,6 +373,7 @@ fn processTypeInstruction(self: *Assembler) !AsmValue { /// - Function-local instructions are emitted in `self.func`. fn processGenericInstruction(self: *Assembler) !?AsmValue { const operands = self.inst.operands.items; + var maybe_spv_decl_index: ?SpvModule.Decl.Index = null; const section = switch (self.inst.opcode.class()) { .ConstantCreation => &self.spv.sections.types_globals_constants, .Annotation => &self.spv.sections.annotations, @@ -378,12 +383,15 @@ fn processGenericInstruction(self: *Assembler) !?AsmValue { .OpExecutionMode, .OpExecutionModeId => &self.spv.sections.execution_modes, .OpVariable => switch (@as(spec.StorageClass, @enumFromInt(operands[2].value))) { .Function => &self.func.prologue, - .UniformConstant => &self.spv.sections.types_globals_constants, - else => { - // This is currently disabled because global variables are required to be - // emitted in the proper order, and this should be honored in inline assembly - // as well. - return self.todo("global variables", .{}); + // These don't need to be marked in the dependency system. + // Probably we should add them anyway, then filter out PushConstant globals. + .PushConstant => &self.spv.sections.types_globals_constants, + else => section: { + maybe_spv_decl_index = try self.spv.allocDecl(.global); + try self.func.decl_deps.put(self.spv.gpa, maybe_spv_decl_index.?, {}); + // TODO: In theory this can be non-empty if there is an initializer which depends on another global... + try self.spv.declareDeclDeps(maybe_spv_decl_index.?, &.{}); + break :section &self.spv.sections.types_globals_constants; }, }, // Default case - to be worked out further. @@ -409,7 +417,10 @@ fn processGenericInstruction(self: *Assembler) !?AsmValue { section.writeDoubleWord(dword); }, .result_id => { - maybe_result_id = self.spv.allocId(); + maybe_result_id = if (maybe_spv_decl_index) |spv_decl_index| + self.spv.declPtr(spv_decl_index).result_id + else + self.spv.allocId(); try section.ensureUnusedCapacity(self.spv.gpa, 1); section.writeOperand(IdResult, maybe_result_id.?); }, @@ -475,8 +486,8 @@ fn resolveRefId(self: *Assembler, ref: AsmValue.Ref) !IdRef { /// error message has been emitted into `self.errors`. fn parseInstruction(self: *Assembler) !void { self.inst.opcode = undefined; - self.inst.operands.shrinkRetainingCapacity(0); - self.inst.string_bytes.shrinkRetainingCapacity(0); + self.inst.operands.clearRetainingCapacity(); + self.inst.string_bytes.clearRetainingCapacity(); const lhs_result_tok = self.currentToken(); const maybe_lhs_result: ?AsmValue.Ref = if (self.eatToken(.result_id_assign)) blk: { @@ -848,6 +859,8 @@ fn tokenText(self: Assembler, tok: Token) []const u8 { /// Tokenize `self.src` and put the tokens in `self.tokens`. /// Any errors encountered are appended to `self.errors`. fn tokenize(self: *Assembler) !void { + self.tokens.clearRetainingCapacity(); + var offset: u32 = 0; while (true) { const tok = try self.nextToken(offset); From b5301558aecdb1939c203681bf67224260ac6427 Mon Sep 17 00:00:00 2001 From: Robin Voetter Date: Sat, 2 Nov 2024 15:23:27 +0100 Subject: [PATCH 04/10] spirv: make default generic address space for vulkan Function We are not using Private variables. This needs to be cleaned up a bit more, this will happen with the general address space improvements. --- src/codegen/spirv.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/codegen/spirv.zig b/src/codegen/spirv.zig index 02e0d75d18..f00d08910c 100644 --- a/src/codegen/spirv.zig +++ b/src/codegen/spirv.zig @@ -1882,7 +1882,7 @@ const NavGen = struct { else => unreachable, }, .shared => .Workgroup, - .local => .Private, + .local => .Function, .global => switch (target.os.tag) { .opencl => .CrossWorkgroup, .vulkan => .PhysicalStorageBuffer, From 688d7055e3c483249391c66bfe02c084477ad81b Mon Sep 17 00:00:00 2001 From: Robin Voetter Date: Sat, 2 Nov 2024 16:05:06 +0100 Subject: [PATCH 05/10] spirv: assembler hacky constant placeholders --- src/Sema.zig | 3 +- src/codegen/spirv.zig | 59 ++++++++++++++++++++++--- src/codegen/spirv/Assembler.zig | 78 ++++++++++++++++++++++++++++++--- 3 files changed, 126 insertions(+), 14 deletions(-) diff --git a/src/Sema.zig b/src/Sema.zig index 771f4a0540..3082cdd3d8 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -17605,6 +17605,8 @@ fn analyzePtrArithmetic( } else break :rs ptr_src; }; + try sema.requireRuntimeBlock(block, op_src, runtime_src); + const target = zcu.getTarget(); if (target_util.arePointersLogical(target, ptr_info.flags.address_space)) { return sema.failWithOwnedErrorMsg(block, msg: { @@ -17623,7 +17625,6 @@ fn analyzePtrArithmetic( }); } - try sema.requireRuntimeBlock(block, op_src, runtime_src); return block.addInst(.{ .tag = air_tag, .data = .{ .ty_pl = .{ diff --git a/src/codegen/spirv.zig b/src/codegen/spirv.zig index f00d08910c..baa37277e8 100644 --- a/src/codegen/spirv.zig +++ b/src/codegen/spirv.zig @@ -6556,13 +6556,59 @@ const NavGen = struct { // for the string, we still use the next u32 for the null terminator. extra_i += (constraint.len + name.len + (2 + 3)) / 4; - if (self.typeOf(input).zigTypeTag(zcu) == .type) { - // This assembly input is a type instead of a value. - // That's fine for now, just make sure to resolve it as such. - const val = (try self.air.value(input, self.pt)).?; - const ty_id = try self.resolveType(val.toType(), .direct); - try as.value_map.put(as.gpa, name, .{ .ty = ty_id }); + const input_ty = self.typeOf(input); + + if (std.mem.eql(u8, constraint, "c")) { + // constant + const val = (try self.air.value(input, self.pt)) orelse { + return self.fail("assembly inputs with 'c' constraint have to be compile-time known", .{}); + }; + + // TODO: This entire function should be handled a bit better... + const ip = &zcu.intern_pool; + switch (ip.indexToKey(val.toIntern())) { + .int_type, + .ptr_type, + .array_type, + .vector_type, + .opt_type, + .anyframe_type, + .error_union_type, + .simple_type, + .struct_type, + .union_type, + .opaque_type, + .enum_type, + .func_type, + .error_set_type, + .inferred_error_set_type, + => unreachable, // types, not values + + .undef => return self.fail("assembly input with 'c' constraint cannot be undefined", .{}), + + .int => { + try as.value_map.put(as.gpa, name, .{ .constant = @intCast(val.toUnsignedInt(zcu)) }); + }, + + else => unreachable, // TODO + } + } else if (std.mem.eql(u8, constraint, "t")) { + // type + if (input_ty.zigTypeTag(zcu) == .type) { + // This assembly input is a type instead of a value. + // That's fine for now, just make sure to resolve it as such. + const val = (try self.air.value(input, self.pt)).?; + const ty_id = try self.resolveType(val.toType(), .direct); + try as.value_map.put(as.gpa, name, .{ .ty = ty_id }); + } else { + const ty_id = try self.resolveType(input_ty, .direct); + try as.value_map.put(as.gpa, name, .{ .ty = ty_id }); + } } else { + if (input_ty.zigTypeTag(zcu) == .type) { + return self.fail("use the 't' constraint to supply types to SPIR-V inline assembly", .{}); + } + const val_id = try self.resolve(input); try as.value_map.put(as.gpa, name, .{ .value = val_id }); } @@ -6624,6 +6670,7 @@ const NavGen = struct { .just_declared, .unresolved_forward_reference => unreachable, .ty => return self.fail("cannot return spir-v type as value from assembly", .{}), .value => |ref| return ref, + .constant => return self.fail("cannot return constant from assembly", .{}), } // TODO: Multiple results diff --git a/src/codegen/spirv/Assembler.zig b/src/codegen/spirv/Assembler.zig index 8fcbad7189..2cfb590273 100644 --- a/src/codegen/spirv/Assembler.zig +++ b/src/codegen/spirv/Assembler.zig @@ -45,6 +45,9 @@ const Token = struct { pipe, /// =. equals, + /// $identifier. This is used (for now) for constant values, like integers. + /// These can be used in place of a normal `value`. + placeholder, fn name(self: Tag) []const u8 { return switch (self) { @@ -56,6 +59,7 @@ const Token = struct { .string => "", .pipe => "'|'", .equals => "'='", + .placeholder => "", }; } }; @@ -128,12 +132,19 @@ const AsmValue = union(enum) { /// This result-value represents a type registered into the module's type system. ty: IdRef, + /// This is a pre-supplied constant integer value. + constant: u32, + /// Retrieve the result-id of this AsmValue. Asserts that this AsmValue /// is of a variant that allows the result to be obtained (not an unresolved /// forward declaration, not in the process of being declared, etc). pub fn resultId(self: AsmValue) IdRef { return switch (self) { - .just_declared, .unresolved_forward_reference => unreachable, + .just_declared, + .unresolved_forward_reference, + // TODO: Lower this value as constant? + .constant, + => unreachable, .value => |result| result, .ty => |result| result, }; @@ -383,16 +394,16 @@ fn processGenericInstruction(self: *Assembler) !?AsmValue { .OpExecutionMode, .OpExecutionModeId => &self.spv.sections.execution_modes, .OpVariable => switch (@as(spec.StorageClass, @enumFromInt(operands[2].value))) { .Function => &self.func.prologue, - // These don't need to be marked in the dependency system. - // Probably we should add them anyway, then filter out PushConstant globals. - .PushConstant => &self.spv.sections.types_globals_constants, - else => section: { + .Input, .Output => section: { maybe_spv_decl_index = try self.spv.allocDecl(.global); try self.func.decl_deps.put(self.spv.gpa, maybe_spv_decl_index.?, {}); // TODO: In theory this can be non-empty if there is an initializer which depends on another global... try self.spv.declareDeclDeps(maybe_spv_decl_index.?, &.{}); break :section &self.spv.sections.types_globals_constants; }, + // These don't need to be marked in the dependency system. + // Probably we should add them anyway, then filter out PushConstant globals. + else => &self.spv.sections.types_globals_constants, }, // Default case - to be worked out further. else => &self.func.body, @@ -665,6 +676,22 @@ fn parseRefId(self: *Assembler) !void { fn parseLiteralInteger(self: *Assembler) !void { const tok = self.currentToken(); + if (self.eatToken(.placeholder)) { + const name = self.tokenText(tok)[1..]; + const value = self.value_map.get(name) orelse { + return self.fail(tok.start, "invalid placeholder '${s}'", .{name}); + }; + switch (value) { + .constant => |literal32| { + try self.inst.operands.append(self.gpa, .{ .literal32 = literal32 }); + }, + else => { + return self.fail(tok.start, "value '{s}' cannot be used as placeholder", .{name}); + }, + } + return; + } + try self.expectToken(.value); // According to the SPIR-V machine readable grammar, a LiteralInteger // may consist of one or more words. From the SPIR-V docs it seems like there @@ -680,6 +707,22 @@ fn parseLiteralInteger(self: *Assembler) !void { fn parseLiteralExtInstInteger(self: *Assembler) !void { const tok = self.currentToken(); + if (self.eatToken(.placeholder)) { + const name = self.tokenText(tok)[1..]; + const value = self.value_map.get(name) orelse { + return self.fail(tok.start, "invalid placeholder '${s}'", .{name}); + }; + switch (value) { + .constant => |literal32| { + try self.inst.operands.append(self.gpa, .{ .literal32 = literal32 }); + }, + else => { + return self.fail(tok.start, "value '{s}' cannot be used as placeholder", .{name}); + }, + } + return; + } + try self.expectToken(.value); const text = self.tokenText(tok); const value = std.fmt.parseInt(u32, text, 0) catch { @@ -756,6 +799,22 @@ fn parseContextDependentNumber(self: *Assembler) !void { fn parseContextDependentInt(self: *Assembler, signedness: std.builtin.Signedness, width: u32) !void { const tok = self.currentToken(); + if (self.eatToken(.placeholder)) { + const name = self.tokenText(tok)[1..]; + const value = self.value_map.get(name) orelse { + return self.fail(tok.start, "invalid placeholder '${s}'", .{name}); + }; + switch (value) { + .constant => |literal32| { + try self.inst.operands.append(self.gpa, .{ .literal32 = literal32 }); + }, + else => { + return self.fail(tok.start, "value '{s}' cannot be used as placeholder", .{name}); + }, + } + return; + } + try self.expectToken(.value); if (width == 0 or width > 2 * @bitSizeOf(spec.Word)) { @@ -903,6 +962,7 @@ fn nextToken(self: *Assembler, start_offset: u32) !Token { string, string_end, escape, + placeholder, } = .start; var token_start = start_offset; var offset = start_offset; @@ -930,6 +990,10 @@ fn nextToken(self: *Assembler, start_offset: u32) !Token { offset += 1; break; }, + '$' => { + state = .placeholder; + tag = .placeholder; + }, else => { state = .value; tag = .value; @@ -945,11 +1009,11 @@ fn nextToken(self: *Assembler, start_offset: u32) !Token { ' ', '\t', '\r', '\n', '=', '|' => break, else => {}, }, - .result_id => switch (c) { + .result_id, .placeholder => switch (c) { '_', 'a'...'z', 'A'...'Z', '0'...'9' => {}, ' ', '\t', '\r', '\n', '=', '|' => break, else => { - try self.addError(offset, "illegal character in result-id", .{}); + try self.addError(offset, "illegal character in result-id or placeholder", .{}); // Again, probably a forgotten delimiter here. break; }, From d35dfc5a3ff48331473ca42b97f3a8cba7d4ad9b Mon Sep 17 00:00:00 2001 From: Robin Voetter Date: Sat, 2 Nov 2024 18:54:34 +0100 Subject: [PATCH 06/10] add storage_buffer address space --- lib/std/Target.zig | 2 +- lib/std/builtin.zig | 1 + src/Sema.zig | 2 +- src/codegen/spirv.zig | 1 + src/target.zig | 2 +- 5 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/std/Target.zig b/lib/std/Target.zig index 298da21187..2283004115 100644 --- a/lib/std/Target.zig +++ b/lib/std/Target.zig @@ -1573,7 +1573,7 @@ pub const Cpu = struct { .fs, .gs, .ss => arch == .x86_64 or arch == .x86, .global, .constant, .local, .shared => is_gpu, .param => is_nvptx, - .input, .output, .uniform, .push_constant => is_spirv, + .input, .output, .uniform, .push_constant, .storage_buffer => is_spirv, // TODO this should also check how many flash banks the cpu has .flash, .flash1, .flash2, .flash3, .flash4, .flash5 => arch == .avr, diff --git a/lib/std/builtin.zig b/lib/std/builtin.zig index 02948d6602..ed18965e50 100644 --- a/lib/std/builtin.zig +++ b/lib/std/builtin.zig @@ -515,6 +515,7 @@ pub const AddressSpace = enum(u5) { output, uniform, push_constant, + storage_buffer, // AVR address spaces. flash, diff --git a/src/Sema.zig b/src/Sema.zig index 3082cdd3d8..30f033b0a1 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -37852,7 +37852,7 @@ pub fn analyzeAsAddressSpace( .gs, .fs, .ss => (arch == .x86 or arch == .x86_64) and ctx == .pointer, // TODO: check that .shared and .local are left uninitialized .param => is_nv, - .input, .output, .uniform, .push_constant => is_spirv, + .input, .output, .uniform, .push_constant, .storage_buffer => is_spirv, .global, .shared, .local => is_gpu, .constant => is_gpu and (ctx == .constant), // TODO this should also check how many flash banks the cpu has diff --git a/src/codegen/spirv.zig b/src/codegen/spirv.zig index baa37277e8..9197a0959f 100644 --- a/src/codegen/spirv.zig +++ b/src/codegen/spirv.zig @@ -1893,6 +1893,7 @@ const NavGen = struct { .input => .Input, .output => .Output, .uniform => .Uniform, + .storage_buffer => .StorageBuffer, .gs, .fs, .ss, diff --git a/src/target.zig b/src/target.zig index aba1fffcbc..94bb842b12 100644 --- a/src/target.zig +++ b/src/target.zig @@ -458,7 +458,7 @@ pub fn arePointersLogical(target: std.Target, as: AddressSpace) bool { .global => false, // TODO: Allowed with VK_KHR_variable_pointers. .shared => true, - .constant, .local, .input, .output, .uniform, .push_constant => true, + .constant, .local, .input, .output, .uniform, .push_constant, .storage_buffer => true, else => unreachable, }; } From b16252b17eced52f73007a1b28dafb2857054ca4 Mon Sep 17 00:00:00 2001 From: Robin Voetter Date: Sat, 2 Nov 2024 18:57:50 +0100 Subject: [PATCH 07/10] spirv: make all vulkan structs Block for now --- src/codegen/spirv.zig | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/codegen/spirv.zig b/src/codegen/spirv.zig index 9197a0959f..3c4037abc7 100644 --- a/src/codegen/spirv.zig +++ b/src/codegen/spirv.zig @@ -1396,10 +1396,6 @@ const NavGen = struct { const child_ty_id = try self.resolveType(child_ty, child_repr); - if (storage_class == .Uniform or storage_class == .PushConstant) { - try self.spv.decorate(child_ty_id, .Block); - } - try self.spv.sections.types_globals_constants.emit(self.spv.gpa, .OpTypePointer, .{ .id_result = result_id, .storage_class = storage_class, @@ -1746,6 +1742,10 @@ const NavGen = struct { defer self.gpa.free(type_name); try self.spv.debugName(result_id, type_name); + if (target.os.tag == .vulkan) { + try self.spv.decorate(result_id, .Block); // Decorate all structs as block for now... + } + return result_id; }, .struct_type => ip.loadStructType(ty.toIntern()), @@ -1791,6 +1791,10 @@ const NavGen = struct { defer self.gpa.free(type_name); try self.spv.debugName(result_id, type_name); + if (target.os.tag == .vulkan) { + try self.spv.decorate(result_id, .Block); // Decorate all structs as block for now... + } + return result_id; }, .optional => { From 89bd987f1c5f9aed8c7d0f851eae93c7cbf1d70b Mon Sep 17 00:00:00 2001 From: Robin Voetter Date: Sat, 2 Nov 2024 18:58:21 +0100 Subject: [PATCH 08/10] spirv: emit ArrayStride for many-item pointers --- src/codegen/spirv.zig | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/codegen/spirv.zig b/src/codegen/spirv.zig index 3c4037abc7..e305b7ec92 100644 --- a/src/codegen/spirv.zig +++ b/src/codegen/spirv.zig @@ -1639,7 +1639,11 @@ const NavGen = struct { return try self.arrayType(1, elem_ty_id); } else { const result_id = try self.arrayType(total_len, elem_ty_id); - try self.spv.decorate(result_id, .{ .ArrayStride = .{ .array_stride = @intCast(elem_ty.abiSize(zcu)) } }); + if (target.os.tag == .vulkan) { + try self.spv.decorate(result_id, .{ .ArrayStride = .{ + .array_stride = @intCast(elem_ty.abiSize(zcu)), + } }); + } return result_id; } }, @@ -1694,8 +1698,15 @@ const NavGen = struct { .pointer => { const ptr_info = ty.ptrInfo(zcu); + const child_ty = Type.fromInterned(ptr_info.child); const storage_class = self.spvStorageClass(ptr_info.flags.address_space); - const ptr_ty_id = try self.ptrType(Type.fromInterned(ptr_info.child), storage_class); + const ptr_ty_id = try self.ptrType(child_ty, storage_class); + + if (target.os.tag == .vulkan and ptr_info.flags.size == .Many) { + try self.spv.decorate(ptr_ty_id, .{ .ArrayStride = .{ + .array_stride = @intCast(child_ty.abiSize(zcu)), + } }); + } if (ptr_info.flags.size != .Slice) { return ptr_ty_id; From efb7539cb6cd4caf63e17f50c7eace198339460c Mon Sep 17 00:00:00 2001 From: Robin Voetter Date: Sat, 2 Nov 2024 19:01:41 +0100 Subject: [PATCH 09/10] spirv: dont emit forward pointer for annotation instructions --- src/codegen/spirv.zig | 25 ++++++++++++++++++------- src/link/SpirV/deduplicate.zig | 8 ++++++++ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/codegen/spirv.zig b/src/codegen/spirv.zig index e305b7ec92..16b4a6dfbd 100644 --- a/src/codegen/spirv.zig +++ b/src/codegen/spirv.zig @@ -4370,13 +4370,24 @@ const NavGen = struct { defer self.gpa.free(ids); const result_id = self.spv.allocId(); - try self.func.body.emit(self.spv.gpa, .OpInBoundsPtrAccessChain, .{ - .id_result_type = result_ty_id, - .id_result = result_id, - .base = base, - .element = element, - .indexes = ids, - }); + const target = self.getTarget(); + switch (target.os.tag) { + .opencl => try self.func.body.emit(self.spv.gpa, .OpInBoundsPtrAccessChain, .{ + .id_result_type = result_ty_id, + .id_result = result_id, + .base = base, + .element = element, + .indexes = ids, + }), + .vulkan => try self.func.body.emit(self.spv.gpa, .OpPtrAccessChain, .{ + .id_result_type = result_ty_id, + .id_result = result_id, + .base = base, + .element = element, + .indexes = ids, + }), + else => unreachable, + } return result_id; } diff --git a/src/link/SpirV/deduplicate.zig b/src/link/SpirV/deduplicate.zig index f639644f7b..53d79b006f 100644 --- a/src/link/SpirV/deduplicate.zig +++ b/src/link/SpirV/deduplicate.zig @@ -511,6 +511,14 @@ pub fn run(parser: *BinaryModule.Parser, binary: *BinaryModule, progress: std.Pr } if (maybe_result_id_offset == null or maybe_result_id_offset.? != i) { + // Only emit forward pointers before type, constant, and global instructions. + // Debug and Annotation instructions don't need the forward pointer, and it + // messes up the logical layout of the module. + switch (inst.opcode.class()) { + .TypeDeclaration, .ConstantCreation, .Memory => {}, + else => continue, + } + const id: ResultId = @enumFromInt(operand.*); const index = info.entities.getIndex(id) orelse continue; const entity = info.entities.values()[index]; From 9cd7b8359c435a7a0c1309cbf529a100d5422b4f Mon Sep 17 00:00:00 2001 From: Robin Voetter Date: Sat, 2 Nov 2024 19:01:57 +0100 Subject: [PATCH 10/10] spirv: enable variable pointers for now This seems to be required for ptr_elem_ptr with storage buffers. Note that this does not imply that the pointer can be regarded as physical too. Some variants of ptr_elem_ptr will need to be forbidden --- src/link/SpirV.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/link/SpirV.zig b/src/link/SpirV.zig index 106b52a72c..b1b8945963 100644 --- a/src/link/SpirV.zig +++ b/src/link/SpirV.zig @@ -296,7 +296,7 @@ fn writeCapabilities(spv: *SpvModule, target: std.Target) !void { // TODO: Integrate with a hypothetical feature system const caps: []const spec.Capability = switch (target.os.tag) { .opencl => &.{ .Kernel, .Addresses, .Int8, .Int16, .Int64, .Float64, .Float16, .Vector16, .GenericPointer }, - .vulkan => &.{ .Shader, .PhysicalStorageBufferAddresses, .Int8, .Int16, .Int64, .Float64, .Float16 }, + .vulkan => &.{ .Shader, .PhysicalStorageBufferAddresses, .Int8, .Int16, .Int64, .Float64, .Float16, .VariablePointers, .VariablePointersStorageBuffer }, else => unreachable, };