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

Enable integer_atomics feature for Atomic*128 #15

Merged
merged 6 commits into from
Nov 17, 2024
Merged

Conversation

kcking
Copy link
Contributor

@kcking kcking commented Nov 11, 2024

Proposing this change as it fixed the build of rust-cuda for me on windows with rustc channel = "nightly-2024-07-21", though I am not sure if there is a more correct way to solve this.

@juntyr
Copy link
Owner

juntyr commented Nov 11, 2024

Huh, this is curious. Your change seems to require 128b atomics to be enabled for any atomic implementations to work. I don't think that's what we want - would you be able to share the error you got when building rust-cuda? Perhaps I can identify the root issue then :)

@kcking
Copy link
Contributor Author

kcking commented Nov 11, 2024

I believe the root cause is that the integer_atomics feature must be enabled to use AtomicU/I128. AtomicU/I128 isn't present on some platforms, so I am guessing that is why no one has run into this yet. This PR shouldn't limit functionality on any platforms as the integer_atomics feature isn't enabled in any scenarios on main, this PR just enables it if the platform has 128b atomics. A simpler change might be to just always enable the integer_atomics feature, even on platforms where it isn't needed.

error[E0658]: use of unstable library feature 'integer_atomics'
  --> C:\Users\kevin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\const-type-layout-0.3.2\src\impls\core\sync\atomic.rs:9:36
   |
9  |           unsafe impl TypeLayout for core::sync::atomic::$at {
   |                                      ^
...
39 | / impl_atomic_int_layout! {
40 | |     AtomicBool (1:"8") => u8 => false,
41 | |     AtomicI8 (1:"8") => i8 => 0, AtomicI16 (2:"16") => i16 => 0,
42 | |     AtomicI32 (4:"32") => i32 => 0, AtomicI64 (8:"64") => i64 => 0,
...  |
45 | |     AtomicI128 (16:"128") => i128 => 0, AtomicU128 (16:"128") => u128 => 0
46 | | }
   | |_- in this macro invocation
   |
   = note: see issue #99069 <https://github.com/rust-lang/rust/issues/99069> for more information
   = help: add `#![feature(integer_atomics)]` to the crate attributes to enable
   = note: this compiler was built on 2024-07-20; consider upgrading it if it is out of date
   = note: this error originates in the macro `impl_atomic_int_layout` (in Nightly builds, run with -Z macro-backtrace for more info)

@juntyr
Copy link
Owner

juntyr commented Nov 11, 2024

Thanks! I think I'm slowly understanding what's happening - the feature(integer_atomics) is required for Atomic[U|I]128, but because CI doesn't support 128b atomics at all, we never caught this issue before.

There are two possibilities now.

a) Your code clearly fixes the issue by enabling the feature if needed (I misread the change earlier)
b) We could add an additional crate feature, e.g. imps-atomics-128, to gate the Atomic[U|I]128 behind so that one has to additionally opt into using the unstable Atomic[U|I]128 types

src/lib.rs Outdated Show resolved Hide resolved
@juntyr

This comment was marked as outdated.

@juntyr
Copy link
Owner

juntyr commented Nov 13, 2024

@kcking Could you check that this version now works for you (also if you actually use an Atomic[I|U]128 type)?

@kcking
Copy link
Contributor Author

kcking commented Nov 17, 2024

I am actually only using this crate transitively through rust-cuda, in my test case I'm using AtomicU64 and windows. Replacing rust-cuda's version of this crate with cc94bc8 works for me! (I can't test AtomicU128 with rust-cuda as I don't think rust ptx has Atomic*128 support).

@juntyr juntyr merged commit 54da0cf into juntyr:main Nov 17, 2024
6 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
Development

Successfully merging this pull request may close these issues.

2 participants