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

Document GenericMemory and AtomicMemory #54642

Merged
merged 7 commits into from
Jun 10, 2024

Conversation

oscardssmith
Copy link
Member

@oscardssmith oscardssmith commented May 31, 2024

Closes #53854. After talking with @vtjnash, we are ready to commit to the GenericMemory interface. Sorry @nsajko that this took me so long to get around to.

@oscardssmith oscardssmith requested a review from nsajko May 31, 2024 14:36
@oscardssmith oscardssmith added backport 1.11 Change should be backported to release-1.11 docsystem The documentation building system docs This change adds or pertains to documentation and removed docsystem The documentation building system labels May 31, 2024
@oscardssmith oscardssmith mentioned this pull request May 31, 2024
Comment on lines 40 to 51
One-dimensional, fixed-size, dense array with elements of type `T`, where each element is
independently atomic when accessed, and cannot be set non-atomically.
For details, see [Atomic Operations](@ref man-atomic-operations)
Copy link
Contributor

Choose a reason for hiding this comment

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

what does it mean precisely in terms of julia code? man-atomic-operations deals with Threads.Atomic as well as @atomic macro. I presume these sentences relate to the latter. Does that mean that at_mem[3] = 5 is performed atomically (without adding @atomic in front? Same question for x = at_mem[3].

I've been waiting for using this already quite some time but it's really hard without any docs ;) e.g. can a non-isbits x be in a non-consistent state when read from at_mem::AtomicMemory (e.g. some other thread was modifying it at the same time)?
I presume that the whole point of AtomicMemory is that one does not need to use @atomic, but I'd be nice to have it stated in clear text here.

also a tangent question, but the next building block would be a CAS operation on an AtomicMemory, but neither this docstring nor man-atomic-operations mention it (except for Threads.atomic_cas! which deals only with Threads.Atomic).

Copy link
Member Author

Choose a reason for hiding this comment

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

so the issue currently is that the @atomic macro hasn't been updated to support Atomic Memory, but once it is, it will allow you to write
@atomic :release mem[3]=2 for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

does that mean that @atomic macro will be obligatory for reading AtomicMemory? or that it would default to :monotonic? how about writing into AtomicMemory? and of course cas operations: will there be @atomiccas mem[3] 4=>2 ala Atomix.jl?

Copy link
Member Author

Choose a reason for hiding this comment

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

reads default to monotonic. writes require an explicit order. not sure about CAS.

Copy link
Contributor

Choose a reason for hiding this comment

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

@oscardssmith it would be great if this information were included in the docs!

Copy link
Contributor

Choose a reason for hiding this comment

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

@vtjnash thanks for the info. The Manifesto was modified last time one year ago and doesn't even mention AtomicMemory so I was hesitant to rely on the document.

What I understand is that the plan is to implement

  • @atomic ord x[3]Core.memoryrefget(memoryref(x, 3), ord, ...)
  • @atomic ord x[3] = 4Core.memoryrefset!(memoryref(x, 3), 4, ord, ...)
  • @atomicswap ord x[3]Core.memoryrefswap!(memoryref(x, 3), 4, ord, ...)
  • @atomicreplace ord x[3] 3=>4Core.memoryrefreplace!(memoryref(x, 3), 3, 4, ord, ...)

to extend the @atomic[...] macros? (So far these work only on atomic fields). I'd happily give it a try myself if that's the goal ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

also docstrings of Core.memoryref[...] functions mention often isatomic parameter which I infer to be the type parameter of AtomicMemory but isatomic sounds more like function (on Memory?) and is not mentioned at all in the docs elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe that's a remnant of an earlier version of the pr where we had an isatomic bool for the type parameter rather than a symbol kind. time to fix the docs more :)

Copy link
Member Author

Choose a reason for hiding this comment

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

For CAS, we have Core.memoryrefreplace! and Core.memoryrefmodify! which are the implimentations of the (to be created modifyindex! and replaceindex! that function analogously to modifyfield! and replacefield! respectively.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also when we do this, we will want to give CUDA.jl a companion PR that updates https://github.com/JuliaGPU/CUDA.jl/blob/b07cf990bce048e106c7d767ae6003d574706407/src/device/intrinsics/atomics.jl to use the new names.

Comment on lines +16 to +17
const Generic = bitcast(Core.AddrSpace{CUDA}, 0)
const Global = bitcast(Core.AddrSpace{CUDA}, 1)
Copy link
Contributor

@jakobnissen jakobnissen Jun 3, 2024

Choose a reason for hiding this comment

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

Suggested change
const Generic = bitcast(Core.AddrSpace{CUDA}, 0)
const Global = bitcast(Core.AddrSpace{CUDA}, 1)
const Generic = Core.AddrSpace{CUDA}(0x00)
const Global = Core.AddrSpace{CUDA}(0x01)

This code doesn't work since a) bitcast is not public, and b) the integer must be a 1-byte integer for bitcast to work

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was unresolved with the force push.

Copy link
Member Author

Choose a reason for hiding this comment

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

oops. Will re-resolve once #54681 merges.

@KristofferC KristofferC mentioned this pull request Jun 4, 2024
60 tasks
oscardssmith added a commit that referenced this pull request Jun 4, 2024
While writing docs in #54642, I
became dis-satisfied with how `MemoryRef` currently works and since we
are approaching the deadline for changing things around here, made this.

Previously, we had the builtin `memoryref` which constructed a
`GenericMemoryRef` from a `GenericMemory` or from a `GenericMemoryRef`
and an offset. and then we defined constructors `GenericMemoryRef`,
`MemoryRef` etc that provided a nicish interface around the intrinsic.
The problem with this is that people who want to make a
`GenericMemoryRef` don't care what kind of `GenericMemoryRef` they are
making. That choice is defined by the `GemericMemory`. As such, I have
switched it around so that now the intrinsic is named `memoryrefnew`
which frees up `memoryref` to be the function that constructs the
appropriate type of `GenericMemoryRef`.

This could have been done purely on the Base side, but renaming the
intrinsic seems worth it to me since Base/Core use the (new) `memoryref`
a lot and it seems like we should make the experience the same for
internal and external users rather than making `Base` have to work
around a bad name.
@oscardssmith oscardssmith force-pushed the document-GenericMemory branch from aed6011 to 756326d Compare June 4, 2024 21:32
base/genericmemory.jl Outdated Show resolved Hide resolved
base/genericmemory.jl Outdated Show resolved Hide resolved
base/genericmemory.jl Outdated Show resolved Hide resolved
base/genericmemory.jl Outdated Show resolved Hide resolved

One-dimensional dense array with elements of type `T`.
Fixed-size [`DenseVector{T}`](@ref DenseVector).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Fixed-size [`DenseVector{T}`](@ref DenseVector).
A [`DenseVector{T}`](@ref DenseVector). Fixed-size, in the sense that the length of a value of this type doesn't change after construction.

Maybe it'd be better to be more precise regarding "fixed-size"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really see this is clearer. Is there an ambiguity with the original version? Would "constant length" be clearer?

Copy link
Contributor

@nsajko nsajko Jun 5, 2024

Choose a reason for hiding this comment

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

Perhaps the current version is OK. My motivation is to avoid confusion with the StaticArrays.jl-sense of "constant length", where the length is in the type domain.

base/genericmemory.jl Outdated Show resolved Hide resolved
@oscardssmith oscardssmith force-pushed the document-GenericMemory branch from 95cab81 to 9f7959f Compare June 10, 2024 13:41
@oscardssmith
Copy link
Member Author

review addressed. I believe this is ready to merge.

@oscardssmith oscardssmith merged commit 589fd1a into JuliaLang:master Jun 10, 2024
7 checks passed
@oscardssmith oscardssmith deleted the document-GenericMemory branch June 10, 2024 16:45
@oscardssmith
Copy link
Member Author

Note that #54707 (comment) fixes the giant warning that this PR adds that basically says "sorry you can't use AtomicMemory yet", but this warning will likely exist in 1.11 since I don't think we want to backport that PR this late in the release.

KristofferC pushed a commit that referenced this pull request Jun 13, 2024
Closes #53854. After talking
with @vtjnash, we are ready to commit to the `GenericMemory` interface.
Sorry @nsajko that this took me so long to get around to.

---------

Co-authored-by: Marek Kaluba <kalmar@mailbox.org>
Co-authored-by: Neven Sajko <s@purelymail.com>
(cherry picked from commit 589fd1a)
KristofferC pushed a commit that referenced this pull request Jun 13, 2024
While writing docs in #54642, I
became dis-satisfied with how `MemoryRef` currently works and since we
are approaching the deadline for changing things around here, made this.

Previously, we had the builtin `memoryref` which constructed a
`GenericMemoryRef` from a `GenericMemory` or from a `GenericMemoryRef`
and an offset. and then we defined constructors `GenericMemoryRef`,
`MemoryRef` etc that provided a nicish interface around the intrinsic.
The problem with this is that people who want to make a
`GenericMemoryRef` don't care what kind of `GenericMemoryRef` they are
making. That choice is defined by the `GemericMemory`. As such, I have
switched it around so that now the intrinsic is named `memoryrefnew`
which frees up `memoryref` to be the function that constructs the
appropriate type of `GenericMemoryRef`.

This could have been done purely on the Base side, but renaming the
intrinsic seems worth it to me since Base/Core use the (new) `memoryref`
a lot and it seems like we should make the experience the same for
internal and external users rather than making `Base` have to work
around a bad name.

(cherry picked from commit fa038d9)
KristofferC added a commit that referenced this pull request Jun 25, 2024
Backported PRs:
- [x] #54361 <!-- [LBT] Upgrade to v5.9.0 -->
- [x] #54474 <!-- Unalias source from dest in copytrito -->
- [x] #54548 <!-- Fixes for bitcast bugs with LLVM 17 / opaque pointers
-->
- [x] #54191 <!-- make `AbstractPipe` public -->
- [x] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [x] #53356 <!-- Rename at-scriptdir project argument to at-script and
search upwards for Project.toml -->
- [x] #54545 <!-- typeintersect: fix incorrect innervar handling under
circular env -->
- [x] #54586 <!-- Set storage class of julia globals to dllimport on
windows to avoid auto-import weirdness. Forward port of #54572 -->
- [x] #54587 <!-- Accomodate for rectangular matrices in `copytrito!`
-->
- [x] #54617 <!-- CLI: Use `GetModuleHandleExW` to locate libjulia.dll
-->
- [x] #54605 <!-- Allow libquadmath to also fail as it is not available
on all systems -->
- [x] #54634 <!-- Fix trampoline assembly for build on clang 18 on apple
silicon -->
- [x] #54635 <!-- Aggressive constprop in trevc! to stabilize triangular
eigvec -->
- [x] #54645 <!-- ensure we set the right value to gc_first_tid -->
- [x] #54554 <!-- make elsize public -->
- [x] #54648 <!-- Construct LazyString in error paths for tridiag -->
- [x] #54658 <!-- fix missing uuid check on extension when finding the
location of an extension -->
- [x] #54594 <!-- Switch to Pkg mode prompt immediately and load Pkg in
the background -->
- [x] #54669 <!-- Improve error message in inplace transpose -->
- [x] #54671 <!-- Add boundscheck in bindingkey_eq to avoid OOB access
due to data race -->
- [x] #54672 <!-- make: Fix `sed` command for LLVM libraries with no
symbol versioning -->
- [x] #54624 <!-- more precise aliasing checks for SubArray -->
- [x] #54679 <!-- 🤖 [master] Bump the Distributed stdlib from 6a07d98 to
6c7cdb5 -->
- [x] #54604 <!-- Fix tbaa annotation on union selector bytes inside of
structs -->
- [x] #54690 <!-- Fix assertion/crash when optimizing function with dead
basic block -->
- [x] #54704 <!-- LazyString in reinterpretarray error messages -->
- [x] #54718 <!-- fix prepend StackOverflow issue -->
- [x] #54674 <!-- Reimplement dummy pkg prompt as standard prompt -->
- [x] #54737 <!-- LazyString in interpolated error messages involving
types -->
- [x] #54642 <!-- Document GenericMemory and AtomicMemory -->
- [x] #54713 <!-- make: use `readelf` for LLVM symbol version detection
-->
- [x] #54760 <!-- REPL: improve prompt! async function handler -->
- [x] #54606 <!-- fix double-counting and non-deterministic results in
`summarysize` -->
- [x] #54759 <!-- REPL: Fully populate the dummy Pkg prompt -->
- [x] #54702 <!-- lowering: Recognize argument destructuring inside
macro hygiene -->
- [x] #54678 <!-- Don't let setglobal! implicitly create bindings -->
- [x] #54730 <!-- Fix uuidkey of exts in fast path of `require_stdlib`
-->
- [x] #54765 <!-- Handle no-postdominator case in finalizer pass -->
- [x] #54591 <!-- Don't expose guard pages to malloc_stack API consumers
-->
- [x] #54755 <!-- [TOML] remove Dates hack, replace with explicit usage
-->
- [x] #54721 <!-- add try/catch around scheduler to reset sleep state
-->
- [x] #54631 <!-- Avoid concatenating LazyString in setindex! for
triangular matrices -->
- [x] #54322 <!-- effects: add new `@consistent_overlay` macro -->
- [x] #54785
- [x] #54865
- [x] #54815
- [x] #54795
- [x] #54779
- [x] #54837 

Contains multiple commits, manual intervention needed:
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #54649 <!-- Less restrictive copyto! signature for triangular
matrices -->

Non-merged PRs with backport label:
- [ ] #54779 <!-- make recommendation clearer on manifest version
mismatch -->
- [ ] #54739 <!-- finish implementation of upgradable stdlibs -->
- [ ] #54738 <!-- serialization: fix relocatability bug -->
- [ ] #54574 <!-- Make ScopedValues public -->
- [ ] #54457 <!-- Make `String(::Memory)` copy -->
- [ ] #53957 <!-- tweak how filtering is done for what packages should
be precompiled -->
- [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} -->
- [ ] #53286 <!-- Raise an error when using `include_dependency` with
non-existent file or directory -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Jun 25, 2024
oscardssmith added a commit that referenced this pull request Jul 7, 2024
Following the discussion in #54642

Implemented:
- [x] `modifyindex_atomic!`, `swapindex_atomic!`, `replaceindex_atomic!`
for `GenericMemory`
- [x] `getindex_atomic`, `setindex_atomic!`, `setindexonce_atomic!` for
`GenericMemory`
- [x] add support for references in `@atomic` macros
- [x] add support for vararg indices in `@atomic` macros 
- [x] tests
- [x] update docstrings with example usage
- ~[ ] update Atomics section of the manual (?)~
- [x] news

@oscardssmith @vtjnash 

# New `@atomic` transformations implemented here:
```julia
julia> @macroexpand (@atomic a[i1,i2])
:(Base.getindex_atomic(a, :sequentially_consistent, i1, i2))

julia> @macroexpand (@atomic order a[i1,i2])
:(Base.getindex_atomic(a, order, i1, i2))

julia> @macroexpand (@atomic a[i1,i2] = 2.0)
:(Base.setindex_atomic!(a, :sequentially_consistent, 2.0, i1, i2))

julia> @macroexpand (@atomic order a[i1,i2] = 2.0)
:(Base.setindex_atomic!(a, order, 2.0, i1, i2))

julia> @macroexpand (@AtomicSwap a[i1,i2] = 2.0)
:(Base.swapindex_atomic!(a, :sequentially_consistent, 2.0, i1, i2))

julia> @macroexpand (@AtomicSwap order a[i1,i2] = 2.0)
:(Base.swapindex_atomic!(a, order, 2.0, i1, i2))

julia> @macroexpand (@atomic a[i1,i2] += 2.0)
:((Base.modifyindex_atomic!(a, :sequentially_consistent, +, 2.0, i1, i2))[2])

julia> @macroexpand (@atomic order a[i1,i2] += 2.0)
:((Base.modifyindex_atomic!(a, order, +, 2.0, i1, i2))[2])

julia> @macroexpand (@atomiconce a[i1,i2] = 2.0)
:(Base.setindexonce_atomic!(a, :sequentially_consistent, :sequentially_consistent, 2.0, i1, i2))

julia> @macroexpand (@atomiconce o1 o2 a[i1,i2] = 2.0)
:(Base.setindexonce_atomic!(a, o1, o2, 2.0, i1, i2))

julia> @macroexpand (@atomicreplace a[i1,i2] (2.0=>3.0))
:(Base.replaceindex_atomic!(a, :sequentially_consistent, :sequentially_consistent, 2.0, 3.0, i1, i2))

julia> @macroexpand (@atomicreplace o1 o2 a[i1,i2] (2.0=>3.0))
:(Base.replaceindex_atomic!(a, o1, o2, 2.0, 3.0, i1, i2))

```

---------

Co-authored-by: Oscar Smith <oscardssmith@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

the GenericMemory doc string isn't included in the docs
6 participants