Skip to content

[naga hlsl-out, glsl-out] Support atomicCompareExchangeWeak #7658

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

Open
wants to merge 14 commits into
base: trunk
Choose a base branch
from

Conversation

cryvosh
Copy link

@cryvosh cryvosh commented May 2, 2025

Connections
Fixes #7578
Fixes #4364

Description
Adds support for atomicCompareExchangeWeak translation to hlsl and glsl.

Note this does not implement the [allow_uav_condition] loop attribute mentioned here and here.
Not sure the best way to check if the loop termination is based off a UAV read.
Maybe can later borrow logic from the uniformity analyzer?
Dawn doesn't seem to do this either, though maybe it should?
There is a test with a while loop that depends on .exchanged which seems to pass hlsl validation.

Testing
Added / enabled some basic atomicCompareExchangeWeak tests to Naga.
My personal project using atomicCompareExchangeWeak runs fine now on Windows/Nvidia/DX12.
GL backend is untested outside the Naga tests / validation.

Squash or Rebase?
Squash

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@cryvosh cryvosh marked this pull request as draft May 2, 2025 04:46
@cryvosh cryvosh marked this pull request as ready for review May 2, 2025 05:43
@teoxoy
Copy link
Member

teoxoy commented May 21, 2025

Note this does not implement the [allow_uav_condition] loop attribute mentioned here and here.
Not sure the best way to check if the loop termination is based off a UAV read.
Maybe can later borrow logic from the uniformity analyzer?
Dawn doesn't seem to do this either, though maybe it should?
There is a test with a while loop that depends on .exchanged which seems to pass hlsl validation.

I don't see DXC doing anything with the [allow_uav_condition] attribute https://github.com/search?q=repo%3Amicrosoft%2FDirectXShaderCompiler%20HLSLAllowUAVCondition&type=code, maybe it's legacy.

let fun_str = fun.to_hlsl_suffix();
let compare_expr = match *fun {
crate::AtomicFunction::Exchange { compare: Some(cmp) } => Some(cmp),
Copy link
Member

Choose a reason for hiding this comment

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

We need to add cmp to need_bake_expressions to make sure that the expression chain doesn't get written multiple times. See crate::Statement::SubgroupCollectiveOperation for an example.

Copy link
Author

Choose a reason for hiding this comment

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

Inserted cmp immediately after but let me know if there's a better place to do it

Copy link
Member

Choose a reason for hiding this comment

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

Same comment as the GLSL backend, this should go in update_expressions_to_bake.

// we just wrote `InterlockedAdd`, so negate the argument
write!(self.out, "-")?;
crate::AtomicFunction::Exchange {
compare: Some(compare_expr),
Copy link
Member

Choose a reason for hiding this comment

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

compare_expr needs to be added to need_bake_expressions.

Copy link
Author

Choose a reason for hiding this comment

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

I added this logic directly here, but would it be better somewhere else like in update_expressions_to_bake?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it should go in update_expressions_to_bake since it might be too late to register it here.

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.

Internal error in writing type when atomicCompareExchangeWeak is used in compute shader, on GL backend Implement atomicCompareExchangeWeak
2 participants