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

[naga] [msl-out] Add support for metal's atomic_compare_exchange_weak #5675

Closed
wants to merge 3 commits into from

Conversation

9291Sam
Copy link

@9291Sam 9291Sam commented May 7, 2024

Connections

Fixes wgpu/wgpu#5257 and wgpu/wgpu#4364

Description
Adds an implementation of naga::Statement::Atomic {fun: naga::AtomicFunction::Exchange { compare: Some(expr) } } to the msl-out backend.

Testing
Passes tests given by cargo xtask validate msl in file naga/tests/out/msl/atomicCompareExchange.msl

Checklist

  • Run cargo fmt.
  • 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.

@9291Sam 9291Sam requested a review from a team as a code owner May 7, 2024 00:17
9291Sam added a commit to 9291Sam/patinac that referenced this pull request May 7, 2024
@9291Sam 9291Sam changed the title Add support for metal's atomic_compare_exchange_weak [naga] [msl-out] Add support for metal's atomic_compare_exchange_weak May 7, 2024
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.

This needs to be rebased after #5383.

Comment on lines +3418 to +3428
writeln!(
self.out,
r#"
template<typename R, typename T, typename A> R __make_atomic_cas_result(device A* ptr, T cmp_, T v) {{
T cmp = cmp_;
bool exchanged = {NAMESPACE}::atomic_compare_exchange_weak_explicit(ptr, &cmp, v, {NAMESPACE}::memory_order_relaxed, {NAMESPACE}::memory_order_relaxed);
return R{{cmp, exchanged}};
}}
"#
)?;

Copy link
Member

Choose a reason for hiding this comment

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

Please don't just write this definition into every shader whether it's needed or not. We should only generate this code if we actually use it.

You can easily determine whether the module needs this definition by checking whether PredeclaredType::AtomicCompareExchangeWeakResult is present in module.special_types.predeclared_types, since that will always be there, since the AtomicResult expression uses it.

Copy link
Author

Choose a reason for hiding this comment

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

Coolio, I didn't know that existed, I'll look into that once I get a chance!

write!(self.out, ", ")?;
self.put_expression(value, context, true)?;
write!(self.out, ", {NAMESPACE}::memory_order_relaxed)")?;
fn get_msl_non_weak_exchange_name(
Copy link
Member

Choose a reason for hiding this comment

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

There's no reason this needs to be inlined here, where the details are a distraction.

#5383 already moves the function out of msl/mod.rs. I think this PR should leave it where it landed.

@emcee21
Copy link

emcee21 commented Aug 28, 2024

Need this feature, any hope for this PR?

@9291Sam
Copy link
Author

9291Sam commented Aug 28, 2024

Need this feature, any hope for this PR?

Thanks for the reminder, I put it on my calendar, i'll take a crack at it again soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[msl-out] atomic CompareExchange is not implemented
3 participants