Skip to content

Commit

Permalink
Revert "add unsetindex support to more copyto methods (#51760)" (#54332)
Browse files Browse the repository at this point in the history
This reverts commit f0a28e9.

This introduced in general a try catch inside the inner loop for
`copyto!` and it also has performance regression in other cases
#53430.

Since this was added without any tests and "is not-quite-public API" it
seems easiest to just revert it.
This was added for Memory-to-Array and vice versa but dedicated methods
could be added for that if it is desirable

Fixes #53430,
#52070
  • Loading branch information
KristofferC authored May 13, 2024
2 parents b9f68ac + 7e9676d commit 931f6de
Show file tree
Hide file tree
Showing 15 changed files with 45 additions and 280 deletions.
54 changes: 10 additions & 44 deletions base/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1083,45 +1083,28 @@ function copyto_unaliased!(deststyle::IndexStyle, dest::AbstractArray, srcstyle:
if srcstyle isa IndexLinear
# Single-index implementation
@inbounds for i in srcinds
if isassigned(src, i)
dest[i + Δi] = src[i]
else
_unsetindex!(dest, i + Δi)
end
dest[i + Δi] = src[i]
end
else
# Dual-index implementation
i = idf - 1
@inbounds for a in eachindex(src)
i += 1
if isassigned(src, a)
dest[i] = src[a]
else
_unsetindex!(dest, i)
end
@inbounds for a in src
dest[i+=1] = a
end
end
else
iterdest, itersrc = eachindex(dest), eachindex(src)
if iterdest == itersrc
# Shared-iterator implementation
@inbounds for I in iterdest
if isassigned(src, I)
dest[I] = src[I]
else
_unsetindex!(dest, I)
end
for I in iterdest
@inbounds dest[I] = src[I]
end
else
# Dual-iterator implementation
ret = iterate(iterdest)
@inbounds for a in itersrc
@inbounds for a in src
idx, state = ret::NTuple{2,Any}
if isassigned(src, a)
dest[idx] = src[a]
else
_unsetindex!(dest, idx)
end
dest[idx] = a
ret = iterate(iterdest, state)
end
end
Expand Down Expand Up @@ -1150,11 +1133,7 @@ function copyto!(dest::AbstractArray, dstart::Integer,
(checkbounds(Bool, srcinds, sstart) && checkbounds(Bool, srcinds, sstart+n-1)) || throw(BoundsError(src, sstart:sstart+n-1))
src′ = unalias(dest, src)
@inbounds for i = 0:n-1
if isassigned(src′, sstart+i)
dest[dstart+i] = src′[sstart+i]
else
_unsetindex!(dest, dstart+i)
end
dest[dstart+i] = src′[sstart+i]
end
return dest
end
Expand All @@ -1165,7 +1144,7 @@ function copy(a::AbstractArray)
end

function copyto!(B::AbstractVecOrMat{R}, ir_dest::AbstractRange{Int}, jr_dest::AbstractRange{Int},
A::AbstractVecOrMat{S}, ir_src::AbstractRange{Int}, jr_src::AbstractRange{Int}) where {R,S}
A::AbstractVecOrMat{S}, ir_src::AbstractRange{Int}, jr_src::AbstractRange{Int}) where {R,S}
if length(ir_dest) != length(ir_src)
throw(ArgumentError(LazyString("source and destination must have same size (got ",
length(ir_src)," and ",length(ir_dest),")")))
Expand Down Expand Up @@ -1495,20 +1474,7 @@ function _setindex!(::IndexCartesian, A::AbstractArray, v, I::Vararg{Int,M}) whe
r
end

function _unsetindex!(A::AbstractArray, i::Integer...)
@_propagate_inbounds_meta
_unsetindex!(A, map(to_index, i)...)
end

function _unsetindex!(A::AbstractArray{T}, i::Int...) where T
# this provides a fallback method which is a no-op if the element is already unassigned
# such that copying into an uninitialized object generally always will work,
# even if the specific custom array type has not implemented `_unsetindex!`
@inline
@boundscheck checkbounds(A, i...)
allocatedinline(T) || @inbounds(!isassigned(A, i...)) || throw(MethodError(_unsetindex!, (A, i...)))
return A
end
_unsetindex!(A::AbstractArray, i::Integer) = _unsetindex!(A, to_index(i))

"""
parent(A)
Expand Down
7 changes: 1 addition & 6 deletions base/array.jl
Original file line number Diff line number Diff line change
Expand Up @@ -219,12 +219,7 @@ function _unsetindex!(A::Array, i::Int)
@inbounds _unsetindex!(GenericMemoryRef(A.ref, i))
return A
end
function _unsetindex!(A::Array, i::Int...)
@inline
@boundscheck checkbounds(A, i...)
@inbounds _unsetindex!(A, _to_linear_index(A, i...))
return A
end


# TODO: deprecate this (aligned_sizeof and/or elsize and/or sizeof(Some{T}) are more correct)
elsize(::Type{A}) where {T,A<:Array{T}} = aligned_sizeof(T)
Expand Down
6 changes: 0 additions & 6 deletions base/multidimensional.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1612,12 +1612,6 @@ end
end
end

# _unsetindex
@propagate_inbounds function Base._unsetindex!(A::AbstractArray, i::CartesianIndex)
Base._unsetindex!(A, to_indices(A, (i,))...)
return A
end

## permutedims

## Permute array dims ##
Expand Down
19 changes: 0 additions & 19 deletions base/subarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -409,25 +409,6 @@ function isassigned(V::FastSubArray{<:Any, 1}, i::Int)
r
end

function _unsetindex!(V::FastSubArray, i::Int)
@inline
@boundscheck checkbounds(V, i)
@inbounds _unsetindex!(V.parent, _reindexlinear(V, i))
return V
end
function _unsetindex!(V::FastSubArray{<:Any,1}, i::Int)
@inline
@boundscheck checkbounds(V, i)
@inbounds _unsetindex!(V.parent, _reindexlinear(V, i))
return V
end
function _unsetindex!(V::SubArray{T,N}, i::Vararg{Int,N}) where {T,N}
@inline
@boundscheck checkbounds(V, i...)
@inbounds _unsetindex!(V.parent, reindex(V.indices, i)...)
return V
end

IndexStyle(::Type{<:FastSubArray}) = IndexLinear()

# Strides are the distance in memory between adjacent elements in a given dimension
Expand Down
19 changes: 12 additions & 7 deletions stdlib/LinearAlgebra/src/bidiag.jl
Original file line number Diff line number Diff line change
Expand Up @@ -195,14 +195,19 @@ end

#Converting from Bidiagonal to dense Matrix
function Matrix{T}(A::Bidiagonal) where T
B = Matrix{T}(undef, size(A))
if haszero(T) # optimized path for types with zero(T) defined
size(B,1) > 1 && fill!(B, zero(T))
copyto!(view(B, diagind(B)), A.dv)
copyto!(view(B, diagind(B, A.uplo == 'U' ? 1 : -1)), A.ev)
else
copyto!(B, A)
n = size(A, 1)
B = Matrix{T}(undef, n, n)
n == 0 && return B
n > 1 && fill!(B, zero(T))
@inbounds for i = 1:n - 1
B[i,i] = A.dv[i]
if A.uplo == 'U'
B[i,i+1] = A.ev[i]
else
B[i+1,i] = A.ev[i]
end
end
B[n,n] = A.dv[n]
return B
end
Matrix(A::Bidiagonal{T}) where {T} = Matrix{promote_type(T, typeof(zero(T)))}(A)
Expand Down
11 changes: 5 additions & 6 deletions stdlib/LinearAlgebra/src/diagonal.jl
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,11 @@ AbstractMatrix{T}(D::Diagonal{T}) where {T} = copy(D)
Matrix(D::Diagonal{T}) where {T} = Matrix{promote_type(T, typeof(zero(T)))}(D)
Array(D::Diagonal{T}) where {T} = Matrix(D)
function Matrix{T}(D::Diagonal) where {T}
B = Matrix{T}(undef, size(D))
if haszero(T) # optimized path for types with zero(T) defined
size(B,1) > 1 && fill!(B, zero(T))
copyto!(view(B, diagind(B)), D.diag)
else
copyto!(B, D)
n = size(D, 1)
B = Matrix{T}(undef, n, n)
n > 1 && fill!(B, zero(T))
@inbounds for i in 1:n
B[i,i] = D.diag[i]
end
return B
end
Expand Down
6 changes: 2 additions & 4 deletions stdlib/LinearAlgebra/src/structuredbroadcast.jl
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ struct StructuredMatrixStyle{T} <: Broadcast.AbstractArrayStyle{2} end
StructuredMatrixStyle{T}(::Val{2}) where {T} = StructuredMatrixStyle{T}()
StructuredMatrixStyle{T}(::Val{N}) where {T,N} = Broadcast.DefaultArrayStyle{N}()

const StructuredMatrix{T} = Union{Diagonal{T},Bidiagonal{T},SymTridiagonal{T},Tridiagonal{T},LowerTriangular{T},UnitLowerTriangular{T},UpperTriangular{T},UnitUpperTriangular{T}}
for ST in (Diagonal,Bidiagonal,SymTridiagonal,Tridiagonal,LowerTriangular,UnitLowerTriangular,UpperTriangular,UnitUpperTriangular)
const StructuredMatrix = Union{Diagonal,Bidiagonal,SymTridiagonal,Tridiagonal,LowerTriangular,UnitLowerTriangular,UpperTriangular,UnitUpperTriangular}
for ST in Base.uniontypes(StructuredMatrix)
@eval Broadcast.BroadcastStyle(::Type{<:$ST}) = $(StructuredMatrixStyle{ST}())
end

Expand Down Expand Up @@ -133,7 +133,6 @@ fails as `zero(::Tuple{Int})` is not defined. However,
iszerodefined(::Type) = false
iszerodefined(::Type{<:Number}) = true
iszerodefined(::Type{<:AbstractArray{T}}) where T = iszerodefined(T)
iszerodefined(::Type{<:UniformScaling{T}}) where T = iszerodefined(T)

count_structedmatrix(T, bc::Broadcasted) = sum(Base.Fix2(isa, T), Broadcast.cat_nested(bc); init = 0)

Expand All @@ -147,7 +146,6 @@ fzero(::Type{T}) where T = T
fzero(r::Ref) = r[]
fzero(t::Tuple{Any}) = t[1]
fzero(S::StructuredMatrix) = zero(eltype(S))
fzero(::StructuredMatrix{<:AbstractMatrix{T}}) where {T<:Number} = haszero(T) ? zero(T)*I : missing
fzero(x) = missing
function fzero(bc::Broadcast.Broadcasted)
args = map(fzero, bc.args)
Expand Down
33 changes: 15 additions & 18 deletions stdlib/LinearAlgebra/src/tridiag.jl
Original file line number Diff line number Diff line change
Expand Up @@ -135,17 +135,13 @@ function Matrix{T}(M::SymTridiagonal) where T
n = size(M, 1)
Mf = Matrix{T}(undef, n, n)
n == 0 && return Mf
if haszero(T) # optimized path for types with zero(T) defined
n > 2 && fill!(Mf, zero(T))
@inbounds for i = 1:n-1
Mf[i,i] = symmetric(M.dv[i], :U)
Mf[i+1,i] = transpose(M.ev[i])
Mf[i,i+1] = M.ev[i]
end
Mf[n,n] = symmetric(M.dv[n], :U)
else
copyto!(Mf, M)
n > 2 && fill!(Mf, zero(T))
@inbounds for i = 1:n-1
Mf[i,i] = symmetric(M.dv[i], :U)
Mf[i+1,i] = transpose(M.ev[i])
Mf[i,i+1] = M.ev[i]
end
Mf[n,n] = symmetric(M.dv[n], :U)
return Mf
end
Matrix(M::SymTridiagonal{T}) where {T} = Matrix{promote_type(T, typeof(zero(T)))}(M)
Expand Down Expand Up @@ -590,14 +586,15 @@ axes(M::Tridiagonal) = (ax = axes(M.d,1); (ax, ax))

function Matrix{T}(M::Tridiagonal) where {T}
A = Matrix{T}(undef, size(M))
if haszero(T) # optimized path for types with zero(T) defined
size(A,1) > 2 && fill!(A, zero(T))
copyto!(view(A, diagind(A)), M.d)
copyto!(view(A, diagind(A,1)), M.du)
copyto!(view(A, diagind(A,-1)), M.dl)
else
copyto!(A, M)
end
n = length(M.d)
n == 0 && return A
n > 2 && fill!(A, zero(T))
for i in 1:n-1
A[i,i] = M.d[i]
A[i+1,i] = M.dl[i]
A[i,i+1] = M.du[i]
end
A[n,n] = M.d[n]
A
end
Matrix(M::Tridiagonal{T}) where {T} = Matrix{promote_type(T, typeof(zero(T)))}(M)
Expand Down
13 changes: 0 additions & 13 deletions stdlib/LinearAlgebra/test/bidiag.jl
Original file line number Diff line number Diff line change
Expand Up @@ -904,17 +904,4 @@ end
@test mul!(C1, B, sv, 1, 2) == mul!(C2, B, v, 1 ,2)
end

@testset "Matrix conversion for non-numeric and undef" begin
B = Bidiagonal(Vector{BigInt}(undef, 4), fill(big(3), 3), :U)
M = Matrix(B)
B[diagind(B)] .= 4
M[diagind(M)] .= 4
@test diag(B) == diag(M)

B = Bidiagonal(fill(Diagonal([1,3]), 3), fill(Diagonal([1,3]), 2), :U)
M = Matrix{eltype(B)}(B)
@test M isa Matrix{eltype(B)}
@test M == B
end

end # module TestBidiagonal
13 changes: 0 additions & 13 deletions stdlib/LinearAlgebra/test/diagonal.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1298,17 +1298,4 @@ end
@test yadj == x'
end

@testset "Matrix conversion for non-numeric and undef" begin
D = Diagonal(Vector{BigInt}(undef, 4))
M = Matrix(D)
D[diagind(D)] .= 4
M[diagind(M)] .= 4
@test diag(D) == diag(M)

D = Diagonal(fill(Diagonal([1,3]), 2))
M = Matrix{eltype(D)}(D)
@test M isa Matrix{eltype(D)}
@test M == D
end

end # module TestDiagonal
3 changes: 0 additions & 3 deletions stdlib/LinearAlgebra/test/special.jl
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,8 @@ Random.seed!(1)
struct TypeWithZero end
Base.promote_rule(::Type{TypeWithoutZero}, ::Type{TypeWithZero}) = TypeWithZero
Base.convert(::Type{TypeWithZero}, ::TypeWithoutZero) = TypeWithZero()
Base.zero(x::Union{TypeWithoutZero, TypeWithZero}) = zero(typeof(x))
Base.zero(::Type{<:Union{TypeWithoutZero, TypeWithZero}}) = TypeWithZero()
LinearAlgebra.symmetric(::TypeWithoutZero, ::Symbol) = TypeWithoutZero()
LinearAlgebra.symmetric_type(::Type{TypeWithoutZero}) = TypeWithoutZero
Base.copy(A::TypeWithoutZero) = A
Base.transpose(::TypeWithoutZero) = TypeWithoutZero()
d = fill(TypeWithoutZero(), 3)
du = fill(TypeWithoutZero(), 2)
Expand Down
58 changes: 0 additions & 58 deletions stdlib/LinearAlgebra/test/structuredbroadcast.jl
Original file line number Diff line number Diff line change
Expand Up @@ -298,62 +298,4 @@ end
# structured broadcast with function returning non-number type
@test tuple.(Diagonal([1, 2])) == [(1,) (0,); (0,) (2,)]

@testset "broadcast over structured matrices with matrix elements" begin
function standardbroadcastingtests(D, T)
M = [x for x in D]
Dsum = D .+ D
@test Dsum isa T
@test Dsum == M .+ M
Dcopy = copy.(D)
@test Dcopy isa T
@test Dcopy == D
Df = float.(D)
@test Df isa T
@test Df == D
@test eltype(eltype(Df)) <: AbstractFloat
@test (x -> (x,)).(D) == (x -> (x,)).(M)
@test (x -> 1).(D) == ones(Int,size(D))
@test all(==(2), ndims.(D))
@test_throws MethodError size.(D)
end
@testset "Diagonal" begin
@testset "square" begin
A = [1 3; 2 4]
D = Diagonal([A, A])
standardbroadcastingtests(D, Diagonal)
@test sincos.(D) == sincos.(Matrix{eltype(D)}(D))
M = [x for x in D]
@test cos.(D) == cos.(M)
end

@testset "different-sized square blocks" begin
D = Diagonal([ones(3,3), fill(3.0,2,2)])
standardbroadcastingtests(D, Diagonal)
end

@testset "rectangular blocks" begin
D = Diagonal([ones(Bool,3,4), ones(Bool,2,3)])
standardbroadcastingtests(D, Diagonal)
end

@testset "incompatible sizes" begin
A = reshape(1:12, 4, 3)
B = reshape(1:12, 3, 4)
D1 = Diagonal(fill(A, 2))
D2 = Diagonal(fill(B, 2))
@test_throws DimensionMismatch D1 .+ D2
end
end
@testset "Bidiagonal" begin
A = [1 3; 2 4]
B = Bidiagonal(fill(A,3), fill(A,2), :U)
standardbroadcastingtests(B, Bidiagonal)
end
@testset "UpperTriangular" begin
A = [1 3; 2 4]
U = UpperTriangular([(i+j)*A for i in 1:3, j in 1:3])
standardbroadcastingtests(U, UpperTriangular)
end
end

end
8 changes: 0 additions & 8 deletions stdlib/LinearAlgebra/test/tridiag.jl
Original file line number Diff line number Diff line change
Expand Up @@ -859,12 +859,4 @@ end
@test axes(B) === (ax, ax)
end

@testset "Matrix conversion for non-numeric and undef" begin
T = Tridiagonal(fill(big(3), 3), Vector{BigInt}(undef, 4), fill(big(3), 3))
M = Matrix(T)
T[diagind(T)] .= 4
M[diagind(M)] .= 4
@test diag(T) == diag(M)
end

end # module TestTridiagonal
Loading

0 comments on commit 931f6de

Please sign in to comment.