Skip to content

Implement BroadcastStyle using Adapt's unwrap_type #378

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vchuravy
Copy link
Contributor

The goal here is to support GPU accelerated broadcasts of OffsetArrays.
We tried this before and @timholy had some concerns, but with unwrap_type we should
be able to implement this throughout the ecosystem correctly.

One issue for me is that it changes the behavior of a core functionailty depending on the fact if Adapt.jl
is loaded or not, I would advocate to turn Adapt.jl into a proper dependency again.

TODO

  • Decide if Adapt.jl should become a principal dependency again
  • Implement tests (maybe with OpenCL.jl since that doesn't require a GPU)

Copy link

codecov bot commented Apr 13, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.37%. Comparing base (cb299dc) to head (b3a3edc).

Files with missing lines Patch % Lines
ext/OffsetArraysAdaptExt.jl 0.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (cb299dc) and HEAD (b3a3edc). Click for more details.

HEAD has 10 uploads less than BASE
Flag BASE (cb299dc) HEAD (b3a3edc)
13 3
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #378       +/-   ##
===========================================
- Coverage   95.70%   74.37%   -21.34%     
===========================================
  Files           6        6               
  Lines         466      359      -107     
===========================================
- Hits          446      267      -179     
- Misses         20       92       +72     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@timholy
Copy link
Member

timholy commented Apr 13, 2025

@jishnub knows far more about the current state of the package than I do, so I'll defer to him.

For the context on Adapt, my only concern was how it opened everything up for invalidation. Assuming that issue has been resolved, I don't have any objections.

@maleadt
Copy link

maleadt commented Apr 14, 2025

For the context on Adapt, my only concern was how it opened everything up for invalidation

That's been fixed years ago as per your request.

@timholy
Copy link
Member

timholy commented Apr 14, 2025

That was my understanding too. I only brought it up because of @vchuravy's mention of past concerns.

@maleadt
Copy link

maleadt commented Apr 14, 2025

We should probably still be careful, though. unwrap_type is based on the not-so-nice WrappedArray union, https://github.com/JuliaGPU/Adapt.jl/blob/944774bb77d1e4059750f65de155dd7a96476e07/src/wrappers.jl#L72-L120, which has some known issues (and is where an AbstractWrappedArray in Base would come in). I haven't seen it cause invalidations, only some more expensive method insertion when loading packages.

@timholy
Copy link
Member

timholy commented Apr 14, 2025

Yeah, that's a pretty rough Union!

@RainerHeintzmann
Copy link

RainerHeintzmann commented Apr 17, 2025

Referring also to this discussion:
I had a look into the approach that was taken here. And its is not quite so simple, or at least incomplete, if the aim is to make OffsetArrays.jl compatible with CUDA.jl. For example:

julia> using CUDA
julia> a = reshape(cu(collect(1:24)), 2, 3, 4);
julia> sa = OffsetArray(a, (2, 3, 4));
julia> (@view sa[3:4,4:5,5:6]) == a[1:2,1:2,1:2]
ERROR: Scalar indexing is disallowed.

... and supposedly something similar with a ReshapedArray. This means that the broadcasting did not work properly here.
Views in CuArray do not cause scalar indexing. Another issue is

julia> collect(sa)
ERROR: Scalar indexing is disallowed.

These issues can be fixed as shown here, but the disadvantage is that this needs a specific dependency of the extension on CUDA.jl and it is not generically applicable to any wrapped array.

I guess the first problem may be fixed in a better way by implementing extensions to SubArray and ReshapedArray in a similar way. However the other problem seems to stem from some operations in the Julia AbstractArray (?) directly iterating through the array. Is this really faster or more general than implementing these based on the broadcasting mechanism instead?
If so, is there a chance to migrate to broadcasting or implement some mechanism or macro that switches to broadcasting where wanted?

@RainerHeintzmann
Copy link

It seems like the following definition is missing in Base?

function Base.Broadcast.BroadcastStyle(W::Type{<:SubArray}) 
    return Base.Broadcast.BroadcastStyle(Adapt.unwrap_type(W))
end

At least, @which on a view of a custom array without a special rule returns only the AbstractArray definition, which itself returns DefaultArrayStyle{2}()

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.

4 participants