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

Remove unnecessary explicit memory barriers on ARM #14567

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 5 additions & 16 deletions src/atomic.cr
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ struct Atomic(T)
# (e.g. locks) you may try the acquire/release semantics that may be faster on
# some architectures (e.g. X86) but remember that an acquire must be paired
# with a release for the ordering to be guaranteed.
#
# The code generation always enforces the selected memory order, even on
# weak CPU architectures (e.g. ARM32), with the exception of the Relaxed
# memory order where only the operation itself is atomic.
enum Ordering
Relaxed = LLVM::AtomicOrdering::Monotonic
Acquire = LLVM::AtomicOrdering::Acquire
Expand All @@ -29,14 +33,6 @@ struct Atomic(T)
end

# Adds an explicit memory barrier with the specified memory order guarantee.
#
# Atomics on weakly-ordered CPUs (e.g. ARM32) may not guarantee memory order
# of other memory accesses, and an explicit memory barrier is thus required.
#
# Notes:
# - X86 is strongly-ordered and trying to add a fence should be a NOOP;
# - AArch64 guarantees memory order and doesn't need explicit fences in
# addition to the atomics (but may need barriers in other cases).
macro fence(ordering = :sequentially_consistent)
::Atomic::Ops.fence({{ordering}}, false)
end
Expand Down Expand Up @@ -470,18 +466,11 @@ struct Atomic::Flag
# Atomically tries to set the flag. Only succeeds and returns `true` if the
# flag wasn't previously set; returns `false` otherwise.
def test_and_set : Bool
ret = @value.swap(true, :sequentially_consistent) == false
{% if flag?(:arm) %}
Atomic::Ops.fence(:sequentially_consistent, false) if ret
{% end %}
ret
@value.swap(true, :sequentially_consistent) == false
end

# Atomically clears the flag.
def clear : Nil
{% if flag?(:arm) %}
Atomic::Ops.fence(:sequentially_consistent, false)
{% end %}
@value.set(false, :sequentially_consistent)
end
end
13 changes: 0 additions & 13 deletions src/crystal/rw_lock.cr
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ struct Crystal::RWLock
@readers.add(1, :acquire)

if @writer.get(:acquire) == UNLOCKED
{% if flag?(:arm) %}
Atomic.fence(:acquire)
{% end %}
return
end

Expand All @@ -26,9 +23,6 @@ struct Crystal::RWLock
end

def read_unlock : Nil
{% if flag?(:arm) %}
Atomic.fence(:release)
{% end %}
@readers.sub(1, :release)
end

Expand All @@ -40,16 +34,9 @@ struct Crystal::RWLock
while @readers.get(:acquire) != 0
Intrinsics.pause
end

{% if flag?(:arm) %}
Atomic.fence(:acquire)
{% end %}
end

def write_unlock : Nil
{% if flag?(:arm) %}
Atomic.fence(:release)
{% end %}
@writer.set(UNLOCKED, :release)
end
end
6 changes: 0 additions & 6 deletions src/crystal/spin_lock.cr
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,11 @@ class Crystal::SpinLock
Intrinsics.pause
end
end
{% if flag?(:arm) %}
Atomic.fence(:acquire)
{% end %}
{% end %}
end

def unlock
{% if flag?(:preview_mt) %}
{% if flag?(:arm) %}
Atomic.fence(:release)
{% end %}
@m.set(UNLOCKED, :release)
{% end %}
end
Expand Down
9 changes: 0 additions & 9 deletions src/mutex.cr
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,6 @@ class Mutex
@[AlwaysInline]
def lock : Nil
if @state.swap(LOCKED, :acquire) == UNLOCKED
{% if flag?(:arm) %}
Atomic.fence(:acquire)
{% end %}
@mutex_fiber = Fiber.current unless @protection.unchecked?
return
end
Expand All @@ -67,9 +64,6 @@ class Mutex

if @state.get(:relaxed) == UNLOCKED
if @state.swap(LOCKED, :acquire) == UNLOCKED
{% if flag?(:arm) %}
Atomic.fence(:acquire)
{% end %}
@queue_count.sub(1)

@mutex_fiber = Fiber.current unless @protection.unchecked?
Expand All @@ -94,9 +88,6 @@ class Mutex
return false if i == 0
end
end
{% if flag?(:arm) %}
Atomic.fence(:acquire)
{% end %}
true
end

Expand Down
Loading