-
Notifications
You must be signed in to change notification settings - Fork 117
Use Adapt.jl to change storage and element type #2212
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
base: main
Are you sure you want to change the base?
Changes from all commits
8bf41e9
74bff54
f7a88d6
58b2b09
4eda816
d507c62
7c9ad6c
448bf6f
0951f16
ff48351
3eed915
a21c12e
a0ba9c1
40b5f58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ authors = ["Michael Schlottke-Lakemper <[email protected]>", "Gregor | |
version = "0.11.2-DEV" | ||
|
||
[deps] | ||
Adapt = "79e6a3ab-5dfb-504d-930d-738a2a938a0e" | ||
Accessors = "7d9f7c33-5ae7-4f3b-8dc6-eff91059b697" | ||
CodeTracking = "da1fd8a2-8d9e-5ec2-8556-3022fb5608a2" | ||
ConstructionBase = "187b0558-2788-49d3-abe0-74a17ed4e7c9" | ||
|
@@ -63,6 +64,7 @@ TrixiMakieExt = "Makie" | |
TrixiNLsolveExt = "NLsolve" | ||
|
||
[compat] | ||
Adapt = "4" | ||
Accessors = "0.1.36" | ||
CodeTracking = "1.0.5" | ||
ConstructionBase = "1.5" | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -314,4 +314,66 @@ end | |||||||||||||||||||||||||||||
function raw_copy!(c::AbstractContainer, from::Int, destination::Int) | ||||||||||||||||||||||||||||||
raw_copy!(c, c, from, from, destination) | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
# Containers that support multiple storage backends | ||||||||||||||||||||||||||||||
# should be subtypes of this type. | ||||||||||||||||||||||||||||||
# | ||||||||||||||||||||||||||||||
# The first type parameter must be `Array` by default and if | ||||||||||||||||||||||||||||||
# `Adapt.adapt_structure(to, container)` is called then this must be | ||||||||||||||||||||||||||||||
# `typeof(to)`. This is used in downstream code, e.g. for calling | ||||||||||||||||||||||||||||||
# `wrap_array` with an appropriate type after `resize!`ing. | ||||||||||||||||||||||||||||||
abstract type AbstractHeterogeneousContainer{T} <: AbstractContainer end | ||||||||||||||||||||||||||||||
array_type(::AbstractHeterogeneousContainer{T}) where {T} = T | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
# Subtypes must implement a method for these functions | ||||||||||||||||||||||||||||||
function Adapt.adapt_structure(to, ::AbstractHeterogeneousContainer) | ||||||||||||||||||||||||||||||
error("required method not implemented") | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
# For some storage backends like CUDA.jl, empty arrays do seem to simply be | ||||||||||||||||||||||||||||||
# null pointers which can cause `unsafe_wrap` to fail when calling | ||||||||||||||||||||||||||||||
# Adapt.adapt (ArgumentError, see | ||||||||||||||||||||||||||||||
# https://github.com/JuliaGPU/CUDA.jl/blob/v5.4.2/src/array.jl#L212-L229). | ||||||||||||||||||||||||||||||
# To circumvent this, on length zero arrays this allocates | ||||||||||||||||||||||||||||||
# a separate empty array instead of wrapping. | ||||||||||||||||||||||||||||||
# However, since zero length arrays are not used in calculations, | ||||||||||||||||||||||||||||||
# it should be okay if the underlying storage vectors and wrapped arrays | ||||||||||||||||||||||||||||||
# are not the same as long as they are properly wrapped when `resize!`d etc. | ||||||||||||||||||||||||||||||
function unsafe_wrap_or_alloc(to, vec, size) | ||||||||||||||||||||||||||||||
if length(vec) == 0 | ||||||||||||||||||||||||||||||
return similar(vec, size) | ||||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||||
return unsafe_wrap(to, pointer(vec), size) | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
Comment on lines
+342
to
+348
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
To not shadow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fixed the suggestion above. It's not necessary, from my point of view, to avoid shadowing here, but it's fine to be explicit and verbose. |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
struct TrixiAdaptor{Storage, Real} end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
function trixi_adapt(storage, real, x) | ||||||||||||||||||||||||||||||
adapt(TrixiAdaptor{storage, real}(), x) | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
# Custom rules | ||||||||||||||||||||||||||||||
# 1. handling of StaticArrays | ||||||||||||||||||||||||||||||
function Adapt.adapt_storage(::TrixiAdaptor{<:Any, Real}, x::StaticArrays.StaticArray{S, T, N}) where {Real,S,T,N} | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [JuliaFormatter] reported by reviewdog 🐶
Suggested change
|
||||||||||||||||||||||||||||||
StaticArrays.similar_type(x, Real)(x) | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
# 2. Handling of Arrays | ||||||||||||||||||||||||||||||
function Adapt.adapt_storage(::TrixiAdaptor{Storage, Real}, x::AbstractArray{T}) where{Storage, Real, T<:AbstractFloat} | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [JuliaFormatter] reported by reviewdog 🐶
Suggested change
|
||||||||||||||||||||||||||||||
adapt(Storage{Real}, x) | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
function Adapt.adapt_storage(::TrixiAdaptor{Storage, Real}, x::AbstractArray{T}) where {Storage, Real,T<:StaticArrays.StaticArray} | ||||||||||||||||||||||||||||||
adapt(Storage{StaticArrays.similar_type(T, Real)},x) | ||||||||||||||||||||||||||||||
Comment on lines
+367
to
+368
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [JuliaFormatter] reported by reviewdog 🐶
Suggested change
|
||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
function Adapt.adapt_storage(::TrixiAdaptor{Storage, Real}, x::AbstractArray) where{Storage, Real} | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [JuliaFormatter] reported by reviewdog 🐶
Suggested change
|
||||||||||||||||||||||||||||||
adapt(Storage, x) | ||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
# 3. TODO: Should we have a fallback? But that would imply implementing things for NamedTuple again | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
unsafe_wrap_or_alloc(::TrixiAdaptor{Storage}, vec, size) where {Storage} = unsafe_wrap_or_alloc(Storage, vec, size) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
Comment on lines
+377
to
+378
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [JuliaFormatter] reported by reviewdog 🐶
Suggested change
|
||||||||||||||||||||||||||||||
end # @muladd |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,25 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# By default, Julia/LLVM does not use fused multiply-add operations (FMAs). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# Since these FMAs can increase the performance of many numerical algorithms, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# we need to opt-in explicitly. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# See https://ranocha.de/blog/Optimizing_EC_Trixi for further details. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@muladd begin | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#! format: noindent | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# Wraps a Vector of Arrays, forwards `getindex` to the underlying Vector. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# Implements `Adapt.adapt_structure` to allow offloading to the GPU which is | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# not possible for a plain Vector of Arrays. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
struct VecOfArrays{T <: AbstractArray} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
arrays::Vector{T} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Base.getindex(v::VecOfArrays, i::Int) = Base.getindex(v.arrays, i) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Base.IndexStyle(v::VecOfArrays) = Base.IndexStyle(v.arrays) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Base.size(v::VecOfArrays) = Base.size(v.arrays) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Base.length(v::VecOfArrays) = Base.length(v.arrays) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Base.eltype(v::VecOfArrays{T}) where {T} = T | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
function Adapt.adapt_structure(to, v::VecOfArrays) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return VecOfArrays([Adapt.adapt(to, arr) for arr in v.arrays]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
function Base.convert(::Type{<:VecOfArrays}, v::Vector{<:AbstractArray}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
VecOfArrays(v) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+11
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We try to avoid abbreviations unless really necessary for sanity 😬 Also, it would be consistent with the file name. Note, however, that I do not have a super strong opinion here, thus if you deem There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just saw that RecursiveArrayTools.jl seems to already have a If not, then of course we should use a different name for our own data structure, and keeping There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I saw that. I was just wondering whether their data type could (or should) be used instead of rolling our own. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end # @muladd |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -34,6 +34,32 @@ struct LobattoLegendreBasis{RealT <: Real, NNODES, | |||||||
# negative adjoint wrt the SBP dot product | ||||||||
end | ||||||||
|
||||||||
function Adapt.adapt_structure(to, basis::LobattoLegendreBasis) | ||||||||
inverse_vandermonde_legendre = adapt(to, basis.inverse_vandermonde_legendre) | ||||||||
RealT = eltype(inverse_vandermonde_legendre) | ||||||||
|
||||||||
nodes = SVector{<:Any, RealT}(basis.nodes) | ||||||||
weights = SVector{<:Any, RealT}(basis.weights) | ||||||||
inverse_weights = SVector{<:Any, RealT}(basis.inverse_weights) | ||||||||
boundary_interpolation = adapt(to, basis.boundary_interpolation) | ||||||||
derivative_matrix = adapt(to, basis.derivative_matrix) | ||||||||
derivative_split = adapt(to, basis.derivative_split) | ||||||||
derivative_split_transpose = adapt(to,basis.derivative_split_transpose) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [JuliaFormatter] reported by reviewdog 🐶
Suggested change
|
||||||||
derivative_dhat = adapt(to, basis.derivative_dhat) | ||||||||
return LobattoLegendreBasis{RealT, nnodes(basis), typeof(nodes), | ||||||||
typeof(inverse_vandermonde_legendre), | ||||||||
typeof(boundary_interpolation), | ||||||||
typeof(derivative_matrix)}(nodes, | ||||||||
weights, | ||||||||
inverse_weights, | ||||||||
inverse_vandermonde_legendre, | ||||||||
boundary_interpolation, | ||||||||
derivative_matrix, | ||||||||
derivative_split, | ||||||||
derivative_split_transpose, | ||||||||
derivative_dhat) | ||||||||
end | ||||||||
|
||||||||
function LobattoLegendreBasis(RealT, polydeg::Integer) | ||||||||
nnodes_ = polydeg + 1 | ||||||||
|
||||||||
|
@@ -155,6 +181,16 @@ struct LobattoLegendreMortarL2{RealT <: Real, NNODES, | |||||||
reverse_lower::ReverseMatrix | ||||||||
end | ||||||||
|
||||||||
function Adapt.adapt_structure(to, mortar::LobattoLegendreMortarL2) | ||||||||
forward_upper = adapt(to, mortar.forward_upper) | ||||||||
forward_lower = adapt(to, mortar.forward_lower) | ||||||||
reverse_upper = adapt(to, mortar.reverse_upper) | ||||||||
reverse_lower = adapt(to, mortar.reverse_lower) | ||||||||
return LobattoLegendreMortarL2{eltype(forward_upper), nnodes(mortar), typeof(forward_upper), | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [JuliaFormatter] reported by reviewdog 🐶
Suggested change
|
||||||||
typeof(reverse_upper)}(forward_upper, forward_lower, | ||||||||
reverse_upper, reverse_lower) | ||||||||
end | ||||||||
|
||||||||
function MortarL2(basis::LobattoLegendreBasis) | ||||||||
RealT = real(basis) | ||||||||
nnodes_ = nnodes(basis) | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this comment, in particular
What is "this" referring to? To me, the sentence roughly reads like "the first type parameter must by
Array
andArray
must always be the result oftypeof(to)
", which is very likely not the actual intent 😅Could you please clarify a little?