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

add @SpirvType builtin #23326

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

alichraghi
Copy link
Contributor

This adds a new builtin to the language. @SpirvType makes it possible to create a few specific SPIR-V types like OpTypeImage and OpTypeRuntimeArray in a global scope. the usecase can be commonly found in any shader with textures or storage buffers

Resolves #20550

@Snektron Snektron requested a review from andrewrk March 22, 2025 22:20
@mlugg mlugg added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Mar 22, 2025
@mlugg
Copy link
Member

mlugg commented Mar 22, 2025

I don't understand why this is a separate builtin rather than just being a new field in std.builtin.Type. (I'm aware you were having trouble making the compiler happy with that, but that shouldn't impact language decisions!)

The fact that @typeInfo(@SpirvType(...)) == .@"opaque" is quite odd.

Your InternPool.Key.SpirvType implementation is missing any information differentiating distinct @SpirvType calls at the same site, which suggests inadequate testing; try this:

fn SpirvType(info: std.builtin.SpirvType) type {
    return @SpirvType(info);
}
test {
    const T1 = SpirvType(something);
    const T2 = SpirvType(something_else);
    comptime assert(T1 != T2); // <-- this assertion will fail
}

@alichraghi
Copy link
Contributor Author

alichraghi commented Mar 22, 2025

The reason i made it a different builtin is because #10710 is accepted.

The fact that @typeinfo(@SpirvType(...)) == .@"opaque" is quite odd.

these types are stated as opaque in SPIR-V spec and also behaves quite similar (with an exception for allowing the struct {e: @SpirvType(.runtime_array)} case). but i'm not against at adding asmtype.

Your InternPool.Key.SpirvType implementation is missing any information differentiating distinct @SpirvType calls at the same site, which suggests inadequate testing; try this:

oh i thought a builtin call would always result a new zir_index. thanks

@mlugg
Copy link
Member

mlugg commented Mar 22, 2025

i thought a built-in call would always result in a new zir_index

In that case, to avoid confusion, let me just explain what that is for you. zir_index is a TrackedInst.Index, which, for all intents and purposes, is like a tuple of { FileIndex, Zir.Inst.Index }. (The difference compared to literally just storing those two things is that a TrackedInst.Index is stable across incremental updates, even if things move around in the underlying file.) So, zir_index is just a reference to a ZIR instruction, and those references -- like everything in the InternPool -- are intentionally deduplicated.

@Snektron
Copy link
Collaborator

I don't understand why this is a separate builtin rather than just being a new field in std.builtin.Type. (I'm aware you were having trouble making the compiler happy with that, but that shouldn't impact language decisions!)

I don't feel good about adding SPIR-V types to the language because there are quite a few of them, and not all of them are even accepted by the SPIR-V standard. Vendors are allowed to add SPIR-V types, and so choosing a method that ties SPIR-V types to Zig causes a moving target and language bloat with types that shouldn't be there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Assembly backed opaque types
3 participants