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

WIP add support for parsing Op::AtomicLoad in spv frontend #2304

Closed
wants to merge 2 commits into from

Conversation

schell
Copy link

@schell schell commented Apr 15, 2023

@cwfitzgerald here is my first stab at making progress on gfx-rs/wgpu#4489. Am I on the right track? Are there more steps besides parsing?

@schell
Copy link
Author

schell commented Apr 15, 2023

I see here I provided a body that might match OpAtomicLoad, not OpAtomicIAdd...

@schell schell changed the title WIP add support for parsing Op::AtomicIAdd in spv frontend WIP add support for parsing Op::AtomicLoad in spv frontend Apr 17, 2023
@Patryk27
Copy link
Contributor

Patryk27 commented Apr 20, 2023

fwiw, some time ago I tried to implement support for spir-v atomics as well, but I gave up up due to the inconsistency between the atomic models here:

If I remember correctly, spir-v has untyped atomics (i.e. you can atomic-op almost any piece of memory), while wgsl (and thus Naga) requires typed atomics -- and so supporting them on the spir-v frontend would probably require an extra type promotion late-pass where spir-v int & float types detected to be used in atomic context would be "promoted" into their Naga atomic equivalents across the entire program graph.

(since wgsl and/or Metal forbids castings between atomic and non-atomic types.)

Note that I haven't done much development on Naga, so this is just a couple of loose thoughts - maybe my initial approach was totally wrong and that's why it failed 😄

One way or another, I'd love to see spir-v atomics working - rust-gpu supports them (more or less) and I could use some in my shader 😄

@schell
Copy link
Author

schell commented Apr 23, 2023

@Patryk27 Yes, that's exactly the situation as it stands. It's a really big bite to chew because of having to do that type-promotion. Here's the start of a conversation I had with the gang on the matrix channel (it's pretty long) https://app.element.io/#/room/#naga:matrix.org/$_04dSsaJ6O8BFPxfV61v6Wb9I8xYBy9Zrtm4nvF3PGs

@pema99
Copy link

pema99 commented May 26, 2023

You still attempting this or was it just a quick experiment?

@schell
Copy link
Author

schell commented May 29, 2023

Just a quick experiment - I might pick it up again but if anyone else is going to give it a shot I'll defer.

@pema99
Copy link

pema99 commented May 29, 2023

Alright. I'd love to see this come to fruition, but I read a bit up on what is preventing it (the whole untyped atomics vs Atomic thing), and the need for some kind of inference/promotion pass from regular to atomic types seems a bit... daunting for a first time contributor.

@pema99
Copy link

pema99 commented May 29, 2023

In case anyone reads this and is wondering what I am referring to: gpuweb/gpuweb#2377

@cwfitzgerald
Copy link
Member

Hello, thank you for your PR against Naga!

As part of gfx-rs/wgpu#4231, we have moved development of Naga into the wgpu repository in the Naga subfolder. We have transferred all issues, but we are unable to automatically transfer PRs.

As such, please recreate your PR against the wgpu repository. We apologize for the inconvenience this causes, but will make contributing to both projects more streamlined going forward.

We are leaving PRs open, but once they are transferred, please close the original Naga PR.

@schell
Copy link
Author

schell commented Oct 31, 2023

I will retarget my PR at wgpu, thanks :)

@schell schell closed this Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants