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

Rework wrapper type #23

Merged
merged 4 commits into from
Jun 2, 2020
Merged

Rework wrapper type #23

merged 4 commits into from
Jun 2, 2020

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Jun 2, 2020

No description provided.

@maleadt maleadt merged commit a62a256 into master Jun 2, 2020
@maleadt maleadt deleted the tb/wrappers branch June 2, 2020 07:44
# https://github.com/JuliaLang/julia/pull/31563

# accessors for extracting information about the wrapper type
Base.ndims(::Type{<:WrappedArray{T,N,Src,Dst}}) where {T,N,Src,Dst} = @isdefined(N) ? N : ndims(Dst)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type-piracy is unfortunate. @jakebolewski and I just hunted this down as the source of breaking GPU inference, because Julia doesn't inline this function (might be the unused Src type parameter, e.g. dangling type parameter).

why is the @defined needed here?

x-ref: CliMA/ClimateMachine.jl#1260 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

julia> f(a) = eltype(a)
f (generic function with 1 method)

julia> @code_typed f(B)
CodeInfo(
1 ─     return Float64
) => Type{Float64}

julia> using Adapt
[ Info: Precompiling Adapt [79e6a3ab-5dfb-504d-930d-738a2a938a0e]

julia> @code_typed f(B)
CodeInfo(
1 ─ %1 = invoke Main.eltype(_2::SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true})::Core.Compiler.Const(Float64, false)
└──      return %1
) => Type{Float64}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the @defined needed here?

julia> x(::Base.ReshapedArray{Float32,2,Array{Float32,2}})
ERROR: syntax: invalid "::" syntax around REPL[3]:1
Stacktrace:
 [1] top-level scope at REPL[3]:1

julia> x(Base.ReshapedArray{Float32,2,Array{Float32,2}})
(Float32, 2)

julia> x(LinearAlgebra.Adjoint{Float32,Array{Float32,2}})
ERROR: UndefVarError: N not defined
Stacktrace:
 [1] x(::Type{Adjoint{Float32,Array{Float32,2}}}) at ./REPL[2]:1
 [2] top-level scope at REPL[8]:1

For types that 'fix' the N typevar I can't get a hold of it from the WrappedArray union...
It's all a bit of a kludge, admittedly, but it brings us closer to trying out something that might end up in Base some day.

Suggested fix? Adapt.eltype and Adapt.ndims?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had not seen #26

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think making these Adapt.eltype would make sense

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.

2 participants