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 Threads.Atomic{Bool} #26542

Closed
NHDaly opened this issue Mar 20, 2018 · 4 comments
Closed

Support Threads.Atomic{Bool} #26542

NHDaly opened this issue Mar 20, 2018 · 4 comments
Labels
multithreading Base.Threads and related functionality

Comments

@NHDaly
Copy link
Member

NHDaly commented Mar 20, 2018

Could we add Bool to the list of types supported by Threads.Atomic? Currently atomics only supports int types and float types, but I don't see any reason why it couldn't also support booleans! :)

For now as a workaround, you have to use an x = Threads.Atomic{UInt8} and always check if x != 0 instead of just if x.

I couldn't find either a github issue or discourse thread about this already, sorry if this is a duplicate. Thanks! :)

@ararslan ararslan added the multithreading Base.Threads and related functionality label Mar 20, 2018
@NHDaly
Copy link
Member Author

NHDaly commented Mar 20, 2018

Oh, actually, I just came across this comment when checking to see how atomics are implemented:

# TODO: Support Bool, Ptr
(# TODO: Support Bool, Ptr)

So I guess this is an already-desired TODO. I didn't find an Issue for it, but sorry if this is a duplicate.


That said, would implementing this be more complicated than simply adding Bool to the list of atomictypes? That is, changing this line:

const atomictypes = (inttypes..., floattypes...)

to const atomictypes = (inttypes..., floattypes..., Bool)?

@NHDaly
Copy link
Member Author

NHDaly commented Mar 20, 2018

Ah, so I guess one complication would be deciding what to do about the arithmetic atomic_* functions.

For example, what should atomic_add!(x::Atomic{Bool}, val::Bool) even mean? My gut says it simply shouldn't be defined. But that will mean at least a slight refactoring of the type and its functions...

We could define ArithmeticTypes = Union{(inttypes..., floattypes...)...}, and change those function definitions to atomic_add!(x::Atomic{T}, val::T) where T<:ArithmeticTypes? Does that seem like an okay approach?

NHDaly added a commit to NHDaly/julia that referenced this issue Mar 23, 2018
Adds `Bool` to list of types supported by `Atomic{T}`.

Defines all `atomic_*!` for `Bool`, except `atomic_add!` and `atomic_sub!`
since `add(::Bool, ::Bool)` returns an `Int`.

Also adds tests for those methods to `test/threads.jl`.
NHDaly added a commit to NHDaly/julia that referenced this issue Mar 23, 2018
Adds `Bool` to list of types supported by `Atomic{T}`.

Defines all `atomic_*!` for `Bool`, except `atomic_add!` and `atomic_sub!`
since `add(::Bool, ::Bool)` returns an `Int`.

Also adds tests for those methods to `test/threads.jl`.
NHDaly added a commit to NHDaly/julia that referenced this issue Mar 23, 2018
Adds `Bool` to list of types supported by `Atomic{T}`.

Defines all `atomic_*!` for `Bool`, except `atomic_add!` and `atomic_sub!`
since `add(::Bool, ::Bool)` returns an `Int`.

Also adds tests for those methods to `test/threads.jl`.
@kpamnany
Copy link
Contributor

kpamnany commented Apr 3, 2018

Nice work, appreciated!

@NHDaly
Copy link
Member Author

NHDaly commented Apr 4, 2018

haha thanks. in the PR, I just basically did what i described here in the issue: added where T<:ArithmeticTypes to atomic_add and atomic_sub.

JeffBezanson pushed a commit that referenced this issue Apr 16, 2018
Adds `Bool` to list of types supported by `Atomic{T}`.

Defines all `atomic_*!` for `Bool`, except `atomic_add!` and `atomic_sub!`
since `add(::Bool, ::Bool)` returns an `Int`.

Also adds tests for those methods to `test/threads.jl`.
mbauman added a commit that referenced this issue Apr 19, 2018
* origin/master: (22 commits)
  separate `isbitstype(::Type)` from `isbits` (#26850)
  bugfix for regex matches ending with non-ASCII (#26831)
  [NewOptimizer] track inbounds state as a per-statement flag
  change default LOAD_PATH and DEPOT_PATH (#26804, fix #25709)
  Change url scheme to https (#26835)
  [NewOptimizer] inlining: Refactor todo object
  inference: enable CodeInfo method_for_inference_limit_heuristics support (#26822)
  [NewOptimizer] Fix _apply elision (#26821)
  add test case from issue #26607, cfunction with no args (#26838)
  add `do` in front-end deparser. fixes #17781 (#26840)
  Preserve CallInst metadata in LateLowerGCFrame pass.
  Improve differences from R documentation (#26810)
  reserve syntax that could be used for computed field types (#18466) (#26816)
  Add support for Atomic{Bool} (Fix #26542). (#26597)
  Remove argument restriction on dims2string and inds2string (#26799) (#26817)
  remove some unnecessary `eltype` methods (#26791)
  optimize: ensure merge_value_ssa doesn't drop PiNodes
  inference: improve tmerge for Conditional and Const
  ensure more iterators stay type-stable
  code loading docs (#26787)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multithreading Base.Threads and related functionality
Projects
None yet
Development

No branches or pull requests

3 participants