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

Add support for Atomic(Bool) #14532

Merged
merged 5 commits into from
Apr 28, 2024

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Apr 23, 2024

Tries to enable Atomic(Bool) that should be possible since Bool is represented as an int1 during codegen.

The issue is that atomic ops need at least 1 byte, thus operate on an int8 (or more). A simple solution would be to make @value an Int8 when T is Bool, but while doing so the compiler becomes incapable to infer the type of @value anymore (see #7975) + we can't set @value : {{ T == Bool ? Int8 : T }} at the class level because T is unknown at this point.

This patch thus relies on a hack I'm not sure if it's clever or lucky or plain stupid (it's probably a mix of each):

We may represent Bool as an int1 but it looks like the generated assembly acts on a byte (int8). In fact sizeof(Bool) == alignof(Bool) == 1. We also wrap @value in a struct which means that sizeof/alignof will also be 1 byte anyway, so it seems safe to cast @value : Bool as an int8 pointer pointerof(@value).as(Int8*): there's enough padding to operate on the whole byte (not just the one bit).

Again: it works, but that sounds fragile, if not plain bad....

TODO:

  • spec that uses a Foo type with multiple Atomic(Bool) + intertwined Atomic(Int8) and Atomic(Int16) and verify they don't corrupt each other.

@ysbaddaden ysbaddaden self-assigned this Apr 23, 2024
@HertzDevil
Copy link
Contributor

What does it do that Atomic::Flag cannot achieve?

src/atomic.cr Outdated Show resolved Hide resolved
@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Apr 23, 2024

@HertzDevil Atomic::Flag only supports SWAP(1) == 0 and STORE(0) and is a kind of lock, not a more generic atomic with LOAD, STORE(T/F), CAS(T/F, T/F) and SWAP(T/F).

@ysbaddaden
Copy link
Contributor Author

C for example has both atomic_flag and atomic_bool: https://en.cppreference.com/w/c/atomic

@HertzDevil
Copy link
Contributor

That still sounds like duplicate functionality. Does anything prevent us from implementing Atomic::Flag in terms of Atomic(Bool) or vice versa?

@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Apr 24, 2024

The difference is that Atomic(Bool) is an atomic value, while Atomic::Flag is a kind of lock. Of course Atomic::Flag can be implemented with an Atomic(Bool), but I'm not convinced by the opposite (flag having all atomic methods).

struct Atomic::Flag
  @value = Atomic(Bool).new(false)

  # try_lock
  def test_and_set : Bool
    @value.swap(true, :acquire) == false
  end

  # unlock
  def clear : Nil
    @value.set(false, :release)
  end
end

Aside: I just noticed that C++20 added wait/notify methods to atomic_flag: https://en.cppreference.com/w/cpp/atomic/atomic_flag
Note: C/C++ specifically state that atomic_flag doesn't have load and store ops (unlike atomic_bool).

src/atomic.cr Outdated Show resolved Hide resolved
Skips a special branch for T=Bool.

Doesn't assume that true/false are represented by 1_i8 / 0_i8, or we should _always_ assume the cast (true/false => 1/0 and 1/0 => true/false).
@straight-shoota straight-shoota added this to the 1.13.0 milestone Apr 27, 2024
@straight-shoota straight-shoota changed the title Support Atomic(Bool) Add support for Atomic(Bool) Apr 28, 2024
@straight-shoota straight-shoota merged commit 5cee8a9 into crystal-lang:master Apr 28, 2024
60 checks passed
Comment on lines 474 to 476
{% if flag?(:arm) %}
Atomic::Ops.fence(:sequentially_consistent, false) if ret
{% end %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be handled by the Atomic class?

ditto below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Sija Good question. I'm thinking more and more that if we define a memory order on an atomic operation and the atomics on weak architectures don't, then we should add the barrier right into the barrier (unless the ordering is relaxed).

It might be inefficient in some cases, but you might want to specialize your algorithm for weak architectures, instead of forcing every usage to have a special case for ARM (and not support other weak architectures if we eventually add them).

@ysbaddaden ysbaddaden deleted the feature/atomic-bool branch April 30, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants