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

Atomic64 fixes #5952

Merged
merged 6 commits into from
Jul 14, 2024
Merged

Atomic64 fixes #5952

merged 6 commits into from
Jul 14, 2024

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Jul 13, 2024

Connections

Description

  • Apply fix from issue written by @jimblandy
  • On Vulkan, check for atomic64 PHD feature when inspecting the instance to get supported adapter extensions (the code for vulkan features/extensions in wgpu is seriously confusing, and idk if PhysicalDeviceShaderAtomicInt64Features::default() is correct here, but it seems to work and follow the existing code pattern)
  • On Vulkan, also request shader_shared_int64_atomics in addition to the buffer variant
  • In the HLSL backend, write e.g. InterlockedMax64 instead of InterlockedMax when performing an atomic operation on a buffer

Testing

  • Bevy shaders

Checklist

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

@JMS55 JMS55 requested review from a team as code owners July 13, 2024 19:30
@jimblandy
Copy link
Member

@JMS55 So, why wasn't tests/in/atomicOps-int64-min-max.wgsl broken by the absence of the fix in naga/src/front/wgsl/lower/mod.rs?

@JMS55
Copy link
Contributor Author

JMS55 commented Jul 13, 2024

I couldn't figure that out. My best guess is that maybe the fact that the second argument is not an expression matters. Maybe a constant vs expression in the second argument changes how things get parsed or something.

@jimblandy
Copy link
Member

jimblandy commented Jul 13, 2024

The clippy warnings are specific to this branch, they don't happen on trunk. I can see them with:

cargo +1.76 clippy -p naga --features=hlsl-out

I would like to have a snapshot test that is affected by the change to naga/src/front/wgsl/lower/mod.rs. I'm uncomfortable that we have no tests that can detect that fix.

@JMS55
Copy link
Contributor Author

JMS55 commented Jul 13, 2024

I didn't change that code though, so idk why clippy is complaining only on this branch... I mean I can make the fixes as part of this PR if you want, up to you.

@jimblandy
Copy link
Member

I didn't change that code though, so idk why clippy is complaining only on this branch... I mean I can make the fixes as part of this PR if you want, up to you.

Yes, please change this PR so it does not break CI on trunk when it is merged.

@jimblandy
Copy link
Member

I would like to have a snapshot test that is affected by the change to naga/src/front/wgsl/lower/mod.rs. I'm uncomfortable that we have no tests that can detect that fix.

I volunteered to work on this in chat.

@jimblandy
Copy link
Member

@JMS55 So, why wasn't tests/in/atomicOps-int64-min-max.wgsl broken by the absence of the fix in naga/src/front/wgsl/lower/mod.rs?

It seems like the reason is, the final operands to the atomicMax and atomicMin calls in the snapshot are all simple expressions that don't need to be emitted. I've pushed a commit that uses a variety of final operands, and that causes a panic without the fix.

@jimblandy jimblandy enabled auto-merge (squash) July 14, 2024 02:17
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 looks good. I've pushed a commit to address the only nit I had.

@jimblandy jimblandy merged commit 17fcb19 into gfx-rs:trunk Jul 14, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants