Fix JLArray buffer-backed temporaries against JLArrays 0.3.2 - #296
Open
lkdvos wants to merge 2 commits into
Open
Fix JLArray buffer-backed temporaries against JLArrays 0.3.2#296lkdvos wants to merge 2 commits into
lkdvos wants to merge 2 commits into
Conversation
JLArrays 0.3.2 changed `JLArray`'s `offset` field from a count of elements to a count of bytes, and `pointer` from `pointer(x.data) + x.offset*elsize(x)` to `pointer(x.data) + x.offset`. `unsafe_buffer_wrap` built the temporary with the constructor directly, so under 0.3.2 every buffer-backed temporary landed at byte `start ÷ sizeof(T)` instead of `start`: misaligned, and overlapping both each other and earlier temporaries, which silently corrupted results. Go through `GPUArrays.derive` instead, the documented backend hook for producing an array of a different type and size backed by the same data. Its offset is expressed in elements on both versions, so this is insensitive to how the backend stores it. `ROCArray` already used a byte offset directly and was unaffected. Compat gets a `0.3.1` lower bound, matching the versions this was verified on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`unsafe_buffer_wrap` was written three times, once per backend, and each
reached for a different private detail: `JLArray`'s constructor with an
element offset, `ROCArray`'s with a byte offset, and `unsafe_wrap` on a raw
`CuPtr`. The first of those is what broke against JLArrays 0.3.2.
Route all three through `GPUArrays.derive` in a new `TensorOperationsGPUArraysExt`
instead. It is the documented backend hook for producing an array of a different
type and size backed by the same data, its offset is in elements on every
backend and version, and sharing the storage's refcounted handle keeps the
buffer alive for as long as a temporary derived from it -- which the `CuArray`
path, wrapping a bare pointer, did not do.
`buffer_arraytype` likewise collapses into one method, via a new
`buffer_similartype` that asks the buffer's own storage what it produces for the
requested element type and rank. This resolves at compile time and reproduces
each backend's answer exactly, memory space and buffer type included. It stays
out of the core default because a storage is not always the kind of array it
hands out: `similar(::Memory{UInt8}, T, Dims{1})` is a `Memory`, not a `Vector`.
Since `derive` offsets by whole elements, the element-addressability restriction
that only `JLArray` applied now covers all three, as `buffer_iselementaddressable`.
Element types it rejects fall back on a regular allocation, so this costs buffer
backing for exotic types, never correctness.
Verified against JLArrays 0.3.1 and 0.3.2, so the compat bound stays at "0.3".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes CI on
main.What broke
JLArrays 0.3.2 changed
JLArray'soffsetfield from a count of elements to a count of bytes:offset … in number of elementsoffset … in bytespointerpointer(x.data) + x.offset * Base.elsize(x)pointer(x.data) + x.offsetunsafe_buffer_wrapconstructed the temporary with theJLArrayconstructor directly, passingoffset = Int(start) ÷ sizeof(T). Under 0.3.1 that came back out asbase + start; under 0.3.2 it lands atbase + start ÷ sizeof(T). So every buffer-backed temporary was misaligned and overlapped both its neighbours and earlier temporaries, silently corrupting results — e.g.382943.97where-199.47was expected. 48 failures intest/allocator.jl, reproduced locally.ROCArrayalready passed a byte offset directly and was unaffected.Why CI was green on #295
JLArrays 0.3.2 was released between #295's PR run (15:45 UTC, resolved 0.3.1, green) and its merge run (18:09 UTC, resolved 0.3.2) — and that merge run was cancelled by the next push, so the failure was never surfaced.
JLArrays = "0.3"allowed the upgrade silently.The fix
Use
GPUArrays.derive, the documented backend hook for producing an array of a different type and size backed by the same data (whatreshapeand contiguousviews go through). Itsadditional_offsetis expressed in elements on both 0.3.1 and 0.3.2 — the backend absorbs the representation change — so this is insensitive to how the offset happens to be stored. It also drops the direct use ofGPUArrays.storageplus the private constructor.Verified green on both JLArrays 0.3.1 and 0.3.2 (
test/allocator.jl134/134,test/gpu.jlincluding theCuArraysets). Sincederiveremoves the version sensitivity, compat gets a0.3.1lower bound — matching exactly what was verified — rather than a hard pin to 0.3.2, which would needlessly exclude 0.3.1.The existing tests already catch the regression, so no new ones were added.
Worth reporting the semantics change upstream as well — it is breaking, and shipped in a patch release.
🤖 Generated with Claude Code