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

Image atomics support #6706

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

Image atomics support #6706

wants to merge 1 commit into from

Conversation

atlv24
Copy link
Contributor

@atlv24 atlv24 commented Dec 11, 2024

Connections
Broken off from #5537

Description
Adds image atomics to Vulkan, DirectX, and Metal backends.

Testing
naga snapshot tests and runtime tests are included

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@atlv24 atlv24 requested review from a team as code owners December 11, 2024 22:30
@atlv24 atlv24 mentioned this pull request Dec 11, 2024
6 tasks
@cwfitzgerald cwfitzgerald self-assigned this Dec 11, 2024
@atlv24 atlv24 force-pushed the image-atomics branch 5 times, most recently from 6685477 to d33c0ad Compare December 12, 2024 07:38
Comment on lines -280 to -287
if !storage_access.0.contains(!access) {
frontend.errors.push(Error {
kind: ErrorKind::SemanticError(
"The same memory qualifier can only be used once".into(),
),
meta: token.meta,
})
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what this is for, but if I don't remove it then naga glsl snapshot fails

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started reviewing with the Naga IR, and found something that should be addressed.

naga/src/lib.rs Outdated Show resolved Hide resolved
@jimblandy
Copy link
Member

@atlv24 I appreciate the docs!

@jimblandy

This comment was marked as outdated.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more requests.

naga/src/front/wgsl/lower/mod.rs Outdated Show resolved Hide resolved
naga/src/lib.rs Outdated Show resolved Hide resolved
naga/src/lib.rs Show resolved Hide resolved
@atlv24
Copy link
Contributor Author

atlv24 commented Dec 16, 2024

@jimblandy what did you force push may i ask? and thanks for the review!

@jimblandy
Copy link
Member

@jimblandy what did you force push may i ask? and thanks for the review!

Just changing // to /// to make something a doc comment in the IR, in naga/src/lib.rs.

I always feel bad asking people to change stuff like that because it's trivial, so I just fix it myself, amend to keep the PR review a single commit, and force push. If you'd rather I not amend, that's fine. (And if you'd rather I make lots of minor change requests, I could do that too, but...)

@atlv24
Copy link
Contributor Author

atlv24 commented Dec 16, 2024

nah its fine, was just curious. i can't see the diff and visual inspection yielded nothing

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Validator::validate_entry_point, we have the following code:

let allowed_usage = match var.space {
    ...
    crate::AddressSpace::Private | crate::AddressSpace::WorkGroup => GlobalUse::all(),
    ...
};

By adding ATOMIC to GlobalUse, this became incorrect: atomic accesses to Private address space variables isn't permitted.

I think using atomics in local variables is already covered by Validator::validate_local_var, but validate_entry_point should still not say strange things.

@jimblandy
Copy link
Member

@atlv24 If it's ok, since I've requested some non-trivial changes, rather than finishing this review, I'm going to go review some other, simpler PRs first, and then come back to this one once you've got it revised.

@atlv24
Copy link
Contributor Author

atlv24 commented Dec 16, 2024

@jimblandy thats fine. I am unsure how to remove the sample expression handle from the naga ir, because of the way spir-v wants to write cached expressions. before i added that field, i had tried for quite a while and not found a way to generate it on the fly once, so i settled on the dumb and simple solution.

@jimblandy
Copy link
Member

jimblandy commented Dec 17, 2024

@atlv24 If you call back::spv::Writer::get_constant_null, that should give you a SPIR-V id whose value is zero that you can use as the MS operand. That takes an argument that is a SPIR-V type id, which I think you can get from Writer::get_uint_type_id. Or, if it's easier, you could try Write::get_constant_scalar, something like here.

@atlv24 atlv24 force-pushed the image-atomics branch 2 times, most recently from 26598f2 to 00b45cc Compare December 18, 2024 04:25
@atlv24
Copy link
Contributor Author

atlv24 commented Dec 18, 2024

@jimblandy resolved all feedback

@atlv24 atlv24 requested a review from jimblandy December 18, 2024 04:26
@atlv24 atlv24 force-pushed the image-atomics branch 2 times, most recently from 6c2359a to 5a4b0a8 Compare December 18, 2024 04:31
@cwfitzgerald
Copy link
Member

Self requesting to look at wgpu api side.

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.

3 participants