-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
@extern
: add support for spir-v locations and descriptors
#25506
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
base: master
Are you sure you want to change the base?
Conversation
with the extern changes, they are no longer necessary
flags: Flags, | ||
owner_nav: Nav.Index, | ||
zir_index: TrackedInst.Index, | ||
location_or_descriptor_set: u32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is definitely ugly, haven't seen other examples of large tagged unions in the intern pool so I'm not sure what the preferred pattern here is
I don't know what it would imply but if we replace extern var frag_color: @Vector(4, f32) addrspace(.output = .{ .location = 0 });
extern const ubo: UBO addrspace(.uniform = .{ .set = 0, .binding = 0 }); It would even semantically fix the issue where you can setup a set/binding on an input/output variable like so const out = @extern(*addrspace(.output) Out, .{
.name = "out",
.decoration = .{
.descriptor = .{ .set = 0, .binding = 0 },
},
}); |
Address space and descriptor/binding location are completely different concepts. Moving them into address space would result unintended inequality. I think |
Conceptually yes, but a core tenet of Zig's design is that readability is favored over writability and this is rough to read in a shader. Weren't there discussions about adding more SPIR-V builtins like |
I do agree with @Kbz-8. I believe that adding specific SPIR-V builtin would be beneficial here. Though I think this pr is a great improvement |
I thinks a discussion should be done where everyone sits and speaks about how Zig shaders should be written instead of waiting for potential PRs. |
But this is what pub fn descriptor(T: type, comptime binding: u32, comptime set: u32) T {
return @extern(T, .{
.name = "unused",
.decoration = .{
.descriptor = .{ .binding = binding, .set = set },
}
});
}
const my_image = descriptor(*const Image, 0, 0); though the unused name is a little annoying. I think there are very many potential SPIR-V builtins and I don’t know if Zig wants to go down that rabbit hole. We should be reducing the number of builtins imo, not increasing it. The nice thing about just using (reposing on correct account) |
Feel free to submit a proposal. :) |
The intent of this PR is to allow specifying SPIR-V bindings without hacky inline assembly.
SPIR-V descriptors and locations can now be specified like this:
That is, using the
@extern
builtin.There are a couple unresolved questions:
ExternOptions.name
is meaningless and does nothing. It would be nice if specifying this name was optional. Happy to make this change as well, though we'd have to decide what that means for other targets (e.g., is there a check to make sure it is specified when needed, or should it just inherit from the variable name if it is unspecified, like theextern
keyword?).@extern
values to be pointers, but Zig does. My understanding is that currently we do some sort of magic dereference in the SPIR-V backend. Not sure if it makes sense to relax this requirement in Zig.But overall, even with those questions unresolved, this already provides a much more pleasant GPU programming experience.