-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Expand SPIR-V features #24681
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?
Expand SPIR-V features #24681
Conversation
I strongly believe that we should not support CC @Snektron |
That would require adding them to |
Yes but please wait until #24661 is merged before starting work on this. |
Ah my bad but I not start anything big anyways. Is it okay to start working on update_cpu_features.zig? |
Sure, feel free to work on whatever you like. |
I have now added all capabilities in std.Target.spirv. One important change is the renaming of .arbitrary_precision_integers to .arbitrary_precision_integers_intel. It already had this name in the spirv spec zig file but not in std. I renamed to get consistency with all the new capabilities. Also I'm pretty sure I added all the capabilities at least all from https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_capability. Even the reserved ones, the ones which didn't have their SPIR-V version target stated there I used the version that the extension who implemented the capability required so might not be fully correct? I also stated in the description if the capability should enable an Extension but I did not state which other capabilities it implicitly used this went against how it was implemented before with .variable_pointers but maybe someone else can add it. |
Ah wait I dumb. I now see that https://raw.githubusercontent.com/KhronosGroup/SPIRV-Headers/refs/heads/main/include/spirv/unified1/spirv.core.grammar.json contains the capabilities with their extensions. Okay then that can be automated with gen_spirv_spec and update_cpu_features |
Yes. Note that i reduced the instructions sets to only core, opencl and glsl so don't bother supporting others. |
What do you mean by this? |
That is on your spv4 branch yes? I'm going to add modify gen_spirv_spec soon to generate Extensions and Capabilities which has version and requirements, should I rebase on your fork? |
Yes. Thank you! |
Would I need to move this pr to your fork then? |
I'll continue working on this then, I can rebase once your is merged |
@alichraghi would you know how we can access spec.zig in update_cpu_features.zig? |
Hmm that would be tricky because we now have to pass CC @alexrp TLDR; We need to import |
How would one do it with --dep? So that I can continue working on update_cpu_features.zig will waiting for proper way. |
zig run --dep spirv_spec -Mroot=tools/update_cpu_features.zig -Mspirv_spec=src/codegen/spirv/spec.zig // In update_cpu_features.zig
const spirv_spec = @import("spirv_spec"); |
Thanks, good news is that this is only thing left for it work. I compiled latest commit and it works, it emits correct OpCapability and OpExtension, only stuff that don't work are some capabilities that I did not define in spirv.Feature, from the first commits. But now when update_cpu_features works it should be done |
It should be done, I have yet to test since I'm waiting on compilation. I had to change the design a little bit since there were some limitations with CpuFeatures. But let me know what you think of it now. All Extensions and Capabilities are cpu features and if you add a Capability that requires an Extension or specific version that will be forwarded to codegen. Sadly we can't get dependencies for Extensions since they aren't even in the json file we use for spirv spec, I get them by checking all the dependencies of the capabilities in the spec lol. So using an Extension that requires another one(Don't even know if that exists) it will not add the other one. Should I revert the changes in std.gpu and open this again and do another pr for adding stuff to std.gpu or should i keep this as draft? |
From my testing everything seems to be working, only problem is if you try to compile with allfeatures in your extra cpu features. Then it will cause index out of bound in Target.zig since it's not supposed to handle that many features, but I can't really see when you would do this. |
I don't see the need for another PR as long as you can split all these commits into two
This won't be a problem you once my PR is merged since the unnecessary sets are removed. |
Good to know. There's still the problem of how we should deal with update_cpu_features.zig and its dependency on spec.zig, since that is causing the ci to fail |
@alichraghi do you know why openGL does not define bit sizes of C types in std.Target and instead just panics? Should it not have the same as mesa3d? |
Likely because nobody added them and I have no clue what the correct values would be. |
Yes it's because |
a09d0b3
to
3e185a1
Compare
@alichraghi you know why this still isn't merged? |
Since this touches on |
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.
Do you know how I can reply to your reviews without having to do this?
Just make sure that you don't have an in-progress review, since as you've noticed, GitHub will delay posting your comments until you submit it. If you have in-progress review, you can abandon it to get back to a state where you can just post immediate comments. |
Should I resolve the bit size thing? |
No more comments from me, so I'll hand this off to @alichraghi. |
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.
LGTM! Nice work! I think Sema/InternPool changes need to be reviewed by @mlugg tho.
Currently this adds support for every SPIR-V Extension and Capability. You enable them by setting which ones you want in a featureSet then assigning it to the cpu_features_add in your target. There is also now support for setting some Execution Modes via the calling convention. Following is an example setting the SPIR-V version to 1.6, enabling the SPV_EXT_mesh_shader extension and enabling the MeshShadingEXT capability it also adds the required Execution Modes via the calling convention.
build.zig
shader.zig
shader.spv
As we can see the version is correctly set, the extension is there and the capability as well as the required Execution Modes.