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

Support Atomics #442

Closed
XAMPPRocky opened this issue Feb 22, 2021 · 6 comments
Closed

Support Atomics #442

XAMPPRocky opened this issue Feb 22, 2021 · 6 comments
Labels
a: rust-lang Issues specific to rust-lang/rust. t: enhancement A new feature or improvement to an existing one.

Comments

@XAMPPRocky
Copy link
Member

Currently if you try to use an atomic you get the following error.

Code

Playground

#![no_std]
#![feature(register_attr)]
#![register_attr(spirv)]

use core::sync::atomic::AtomicI32;

#[allow(unused_attributes)]
#[spirv(vertex)]
pub fn main_vs() {
    let atomic = AtomicI32::from(65);
    
    atomic.fetch_add(35, core::sync::atomic::Ordering::AcqRel);
    assert!(atomic.load(core::sync::atomic::Ordering::AcqRel) == 100);
}

Error

error: Cannot use AtomicOrdering=SequentiallyConsistent on Vulkan memory model. Check if AcquireRelease fits your needs.

error: Cannot use AtomicOrdering=SequentiallyConsistent on Vulkan memory model. Check if AcquireRelease fits your needs.

error: Cannot cast between pointer types
     |
     = note: from: *{Function} struct core::sync::atomic::AtomicI32 { v: i32 }
     = note: to: *{Function} u32
     = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: Cannot cast between pointer types
  --> D:\local\Temp\2b65e857-11aa-4ed1-a4a1-8dfa7f30591a\lib.rs:12:18
   |
12 |     let atomic = AtomicI32::new(65);
   |                  ^^^^^^^^^^^^^^^^^^
   |
   = note: from: *{Function} struct core::sync::atomic::AtomicI32 { v: i32 }
   = note: to: *{Function} u32
@XAMPPRocky XAMPPRocky added a: rust-lang Issues specific to rust-lang/rust. t: enhancement A new feature or improvement to an existing one. labels Feb 22, 2021
@charles-r-earp
Copy link
Contributor

I was able to implement these here atomic so it looks like atomics do work on the latest version of the compiler. Is there a particular reason atomics have not been merged?

It seems more natural to me to have these be functions as they are here instead of hooking into core::sync::atomic, since that would more or less require that shader inputs be AtomicU32 etc, which makes it hard to be generic over using atomic ops (like for accumulating across workgroups or shader stages) and impls that do not need atomic ops. It's also more consistent with opencl, glsl, cuda, etc which have functions not atomic types.

My particular use case involves atomic accumulations of f32 via atomic_compare_exchange, as well as storing 16 and 8 bit values via a similar mechanism (thus more portable than requiring 16 explicit types).

@khyperia
Copy link
Contributor

Is there a particular reason atomics have not been merged?

We haven't had the time to do proper research/design/evaluation of the various possibility spaces, because it's not a priority for us. For example...

since that would more or less require that shader inputs be AtomicU32 etc, which makes it hard to be generic over using atomic ops

I'm assuming rust had good reasons for creating the AtomicU32 etc. types instead of having them be operations on "regular" scalars (e.g. saftey, etc.). We should make sure we understand those reasons before blasting ahead with the status quo - rust-gpu is supposed to strive to "be as safe as regular rust", not blindly follow the (potentially unsafe) status quo.

@dvc94ch
Copy link
Contributor

dvc94ch commented Jan 12, 2022

it seems to me that atomics/barriers/subgroups haven't made much progress recently due to unanswered design questions. would it make sense to expose these things as intrinsics in spirv-std until rust-gpu gains support for emitting them automagically?

@khyperia
Copy link
Contributor

@dvc94ch Not really - once a path is set down on, it's difficult to re-evaluate and change direction. It takes even more resources to do so, and considering we don't even have enough to do the research in the first place, that's unlikely to happen.

@dvc94ch
Copy link
Contributor

dvc94ch commented Jan 13, 2022

I'm assuming rust had good reasons for creating the AtomicU32 etc. types instead of having them be operations on "regular" scalars (e.g. saftey, etc.). We should make sure we understand those reasons before blasting ahead with the status quo - rust-gpu is supposed to strive to "be as safe as regular rust", not blindly follow the (potentially unsafe) status quo.

@khyperia seems like the vulkan memory model differs from the c++ memory model that rust uses, which would mean that rust-gpu should do it like this too.

Atomic Operations vs Atomic Objects
C++ atomics are exposed via dedicated atomic objects - which somewhat simplifies the memory model; there’s no interaction between atomic and non-atomic accesses to the same memory locations. SPIR-V’s instead has atomic operations which can be performed on regular variables.

The actual effect on the wider memory model is minimal but does mean that atomics must in some cases be synchronized as if they were non-atomic accesses. This leads to two differences in the model:

Atomics accessing the same memory location are only mutually ordered if they are in the same scope and made through the same reference.

If two atomics are not in each other’s Scopes, they must be ordered as if they were non-atomic operations

This ordering is called Scoped Modification Order in place of C++'s Total Modification Order

Accesses performed by atomics are supersets of regular accesses, with the following differences

Availability and visibility operations are performed automatically for the memory locations accessed by the atomic, for stores and loads respectively.

Atomic accesses and their availability and visibility operations are executed "atomically" as a part of the atomic instruction - i.e. do not contribute to data races.

Justification
The evolution of the language has led to this situation, we’ve just defined how it works.

Mapping C++ to SPIR-V
As C++ does not expose aliasing of atomic and non-atomic objects, no mapping is really required - only use atomic operations on atomic objects, and regular operations on regular objects.

@oisyn
Copy link
Contributor

oisyn commented Nov 16, 2022

Since #877 is merged in, I'm closing this. We still need to tackle #898 though.

@oisyn oisyn closed this as completed Nov 16, 2022
@oisyn oisyn removed their assignment Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: rust-lang Issues specific to rust-lang/rust. t: enhancement A new feature or improvement to an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants