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

use style dispatch in materialize #295

Closed
wants to merge 1 commit into from
Closed

Conversation

vchuravy
Copy link
Member

@maleadt
Copy link
Member

maleadt commented Jun 24, 2020

What's the advantage of this? Tests?

@vchuravy
Copy link
Member Author

dest .= expr, the destination can now be anything that promotes to the right broadcast style instead of being a fixed union.

@piever
Copy link

piever commented Feb 20, 2022

It seems like this is actually needed to get nice interoperability between StructArrays broadcast and GPU arrays broadcast, see JuliaArrays/StructArrays.jl#211 (comment). A StructArray whose component are GPU arrays is a good example of a suitable container that is not covered by the current copyto! rule.

This change (plus potentially moving the backend function to a lightweight package like Adapt.jl to avoid taking a dependency here) should allow external packages with custom broadcast (like StructArrays) to interoperate with GPUArrays broadcast.

@vchuravy
Copy link
Member Author

@piever want to take a stab at this? As you can tell I run out if time to work on it shortly after opening the PR

@N5N3
Copy link
Contributor

N5N3 commented Feb 20, 2022

@vchuravy I guess the following is enough?

@inline function materialize!(::Style, dest, bc::Broadcasted) where {Style<:AbstractGPUArrayStyle}
    return _copyto!(dest, instantiate(Broadcasted{Style}(bc.f, bc.args, axes(dest))))
end

@inline Base.copyto!(dest::BroadcastGPUArray, bc::Broadcasted{Nothing}) = _copyto!(dest, bc) # Keep it for ArrayConflict

@inline Base.copyto!(dest::AbstractArray, bc::Broadcasted{<:AbstractGPUArrayStyle}) = _copyto!(dest, bc)

@inline function _copyto!(dest::AbstractArray, bc::Broadcasted)
    axes(dest) == axes(bc) || Broadcast.throwdm(axes(dest), axes(bc))
    isempty(dest) && return dest
    bc′ = Broadcast.preprocess(dest, bc)
    ...

Local test shows no problem. Is it OK to open a new PR?

@piever
Copy link

piever commented Feb 20, 2022

@N5N3, I think it would be great if you could open PRs here and on StructArrays to check that everything works. I confess I am a bit out of my depth on the technicalities of the broadcast machinery.

@maleadt
Copy link
Member

maleadt commented Mar 8, 2022

Superseded by #393 IIUC

@maleadt maleadt closed this Mar 8, 2022
maleadt pushed a commit that referenced this pull request Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants