Skip to content

Refactor MPIPtr cconvert to support Julia 1.14 - #941

Merged
vchuravy merged 18 commits into
JuliaParallel:masterfrom
nhz2:nz/cconvert-refactor
Sep 9, 2026
Merged

vchuravy merged 18 commits into
JuliaParallel:masterfrom
nhz2:nz/cconvert-refactor

Conversation

@nhz2

@nhz2 nhz2 commented Mar 28, 2026 •

Copy link
Copy Markdown
Contributor

Alternative to #935

CC @eschnett @giordano @vchuravy

@nhz2
nhz2 marked this pull request as ready for review March 28, 2026 20:00
@eschnett

Copy link
Copy Markdown
Contributor

One of the main features of ccall and its automatic conversion to pointers is that it's GC safe, i.e. that it holds on to the respective Julia objects while a pointer to the object is in use. I didn't follow the logic that you're introducing, but since this kind of error (losing a GC root) is very difficult to diagnose, I want to recommend to add respective comments to the code, stating which part of the machinery is holding on to objects, which functions are called before objects are converted to pointers, and which are called on pointers.

@nhz2

nhz2 commented Mar 29, 2026

Copy link
Copy Markdown
Contributor Author

I've added some comments using Claude Code

@eschnett

Copy link
Copy Markdown
Contributor

Thanks! The description sounds correct, and I like the ASCII art.

@nhz2

nhz2 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Bump

@KristofferC

Copy link
Copy Markdown
Contributor

Bump, would be nice to have this fixed (+ new release if possible).

@giordano

Copy link
Copy Markdown
Member

I believe we'd need some help with the review, none of us is particularly familiar with what's the intended way to go about it. I definitely prefer this kind of approach over #935 because that felt too ad hoc, but the need for CConvWrapper here is also odd.

@lcw

lcw commented Aug 28, 2026

Copy link
Copy Markdown
Member

We need to support OpenAPI as well.

@nhz2

nhz2 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

We need to support OpenAPI as well.

What do you mean by this?

@lcw

lcw commented Aug 28, 2026

Copy link
Copy Markdown
Member

We need to support OpenAPI as well.

What do you mean by this?

I think we might need to change these lines as well

MPI.jl/ext/OneAPIExt.jl

Lines 7 to 21 in 61cc82e

function Base.cconvert(::Type{MPIPtr}, A::oneAPI.oneArray{T}) where T
A
end
function Base.unsafe_convert(::Type{MPIPtr}, X::oneAPI.oneArray{T}) where T
reinterpret(MPIPtr, Base.unsafe_convert(oneAPI.ZePtr{T}, X))
end
# only need to define this for strided arrays: all others can be handled by generic machinery
function Base.unsafe_convert(::Type{MPIPtr}, V::SubArray{T,N,P,I,true}) where {T,N,P<:oneAPI.oneArray,I}
X = parent(V)
pX = Base.unsafe_convert(oneAPI.ZePtr{T}, X)
pV = pX + ((V.offset1 + V.stride1) - first(LinearIndices(X)))*sizeof(T)
return reinterpret(MPIPtr, pV)
end

@nhz2
nhz2 marked this pull request as draft September 1, 2026 00:22
@nhz2
nhz2 marked this pull request as ready for review September 1, 2026 03:46
@nhz2

nhz2 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

I've updated OneAPIExt.jl and added an intermediate pointer type trait function mpi_ptr_type.

@eschnett

eschnett commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

There is an error in the documentation. Can you add MPI.mpi_ptr_type to the @docs block in buffers.md? This should make the docs build again.

Alternatively, if this function should be private and not be mentioned in the docs, then convert its docstring to a plain comment.

After this change I think this is good to go. I'm really looking forward to see nightly pass CI again!

@nhz2
nhz2 marked this pull request as draft September 8, 2026 19:42
@nhz2
nhz2 marked this pull request as ready for review September 8, 2026 21:07
@nhz2

nhz2 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

I've made the function private.

@vchuravy
vchuravy merged commit 7b1a259 into JuliaParallel:master Sep 9, 2026
63 of 64 checks passed
@vchuravy

vchuravy commented Sep 9, 2026

Copy link
Copy Markdown
Member

@nhz2 thank you for all your work!

@nhz2
nhz2 deleted the nz/cconvert-refactor branch September 9, 2026 14:10
vchuravy added a commit to vchuravy/FixedSizeArrays.jl that referenced this pull request Oct 13, 2026
On Julia v1.14, `Base.cconvert(Ptr{T}, ::SubArray)` returns a
`Base.OffsetCConvert`, which MPI.jl v0.20.27 cannot convert to an `MPIPtr`,
so views of any array (not only `FixedSizeArray`s) fail there. This is fixed
in MPI.jl by JuliaParallel/MPI.jl#941. Detect this by probing a view of an
`Array` and skip the tests of views if it fails.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MEJYUviAUshj5fZt8LTe8A
vchuravy added a commit to vchuravy/FixedSizeArrays.jl that referenced this pull request Oct 13, 2026
MPI.jl v0.20.28 (JuliaParallel/MPI.jl#941) converts any `AbstractArray` to an
`MPIPtr`, including `Memory` and `SubArray`s on Julia v1.14+, so the
host-memory `cconvert`/`unsafe_convert` methods and the view-support probe in
the tests are no longer needed. Delegating `cconvert` to the parent is kept so
that GPU-backed `FixedSizeArray`s use MPI.jl's pointer type for their storage.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
giordano pushed a commit to JuliaArrays/FixedSizeArrays.jl that referenced this pull request Oct 13, 2026
* Add package extension for MPI.jl

Define `MPI.Buffer(::FixedSizeArray)` and the conversion to `MPI.MPIPtr`, so
that `FixedSizeArray`s, and `SubArray`s of them that MPI.jl stores via their
parent, can be used as communication buffers.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MEJYUviAUshj5fZt8LTe8A

* Add PR number to changelog entry

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MEJYUviAUshj5fZt8LTe8A

* Skip MPI tests of views when MPI.jl cannot communicate `SubArray`s

On Julia v1.14, `Base.cconvert(Ptr{T}, ::SubArray)` returns a
`Base.OffsetCConvert`, which MPI.jl v0.20.27 cannot convert to an `MPIPtr`,
so views of any array (not only `FixedSizeArray`s) fail there. This is fixed
in MPI.jl by JuliaParallel/MPI.jl#941. Detect this by probing a view of an
`Array` and skip the tests of views if it fails.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MEJYUviAUshj5fZt8LTe8A

* Require MPI.jl v0.20.28 and rely on its generic `MPIPtr` conversion

MPI.jl v0.20.28 (JuliaParallel/MPI.jl#941) converts any `AbstractArray` to an
`MPIPtr`, including `Memory` and `SubArray`s on Julia v1.14+, so the
host-memory `cconvert`/`unsafe_convert` methods and the view-support probe in
the tests are no longer needed. Delegating `cconvert` to the parent is kept so
that GPU-backed `FixedSizeArray`s use MPI.jl's pointer type for their storage.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.

6 participants