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

Don't read destination indices when copying structured matrices #55322

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Jul 31, 2024

Fixes the following regression introduced in v1.11

julia> using LinearAlgebra

julia> D = Diagonal(rand(4));

julia> T = Tridiagonal(Vector{BigFloat}(undef, 3), Vector{BigFloat}(undef, 4), Vector{BigFloat}(undef, 3))
4×4 Tridiagonal{BigFloat, Vector{BigFloat}}:
 #undef  #undef     ⋅       ⋅ 
 #undef  #undef  #undef     ⋅ 
        #undef  #undef  #undef
               #undef  #undef

julia> copyto!(T, D)
ERROR: UndefRefError: access to undefined reference
Stacktrace:
  [1] getindex
    @ ./essentials.jl:907 [inlined]
  [2] _broadcast_getindex
    @ ./broadcast.jl:644 [inlined]
  [3] _getindex
    @ ./broadcast.jl:675 [inlined]
  [4] _broadcast_getindex
    @ ./broadcast.jl:650 [inlined]
  [5] getindex
    @ ./broadcast.jl:610 [inlined]
  [6] macro expansion
    @ ./broadcast.jl:973 [inlined]
  [7] macro expansion
    @ ./simdloop.jl:77 [inlined]
  [8] copyto!
    @ ./broadcast.jl:972 [inlined]
  [9] copyto!
    @ ./broadcast.jl:925 [inlined]
 [10] materialize!
    @ ./broadcast.jl:883 [inlined]
 [11] materialize!
    @ ./broadcast.jl:880 [inlined]
 [12] _copyto_banded!(T::Tridiagonal{BigFloat, Vector{BigFloat}}, D::Diagonal{Float64, Vector{Float64}})
    @ LinearAlgebra ~/.julia/juliaup/julia-nightly/share/julia/stdlib/v1.12/LinearAlgebra/src/special.jl:323
 [13] copyto!(dest::Tridiagonal{BigFloat, Vector{BigFloat}}, src::Diagonal{Float64, Vector{Float64}})
    @ LinearAlgebra ~/.julia/juliaup/julia-nightly/share/julia/stdlib/v1.12/LinearAlgebra/src/special.jl:315
 [14] top-level scope
    @ REPL[4]:1

After this PR

julia> copyto!(T, D)
4×4 Tridiagonal{BigFloat, Vector{BigFloat}}:
 0.909968  0.0                  
 0.0       0.193341  0.0         
          0.0       0.194794  0.0
                   0.0       0.506905

The current implementation used an optimization that may not be applicable for non-isbits types, and this PR ensures that we always read from the source and write to the destination.

@jishnub jishnub added linear algebra Linear algebra backport 1.11 Change should be backported to release-1.11 labels Jul 31, 2024
@KristofferC KristofferC mentioned this pull request Aug 2, 2024
68 tasks
@jishnub jishnub requested a review from ViralBShah August 6, 2024 18:33
@jishnub jishnub merged commit fb4e4e5 into master Aug 6, 2024
7 checks passed
@jishnub jishnub deleted the jishnub/copyto_special branch August 6, 2024 19:04
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Aug 17, 2024
…aLang#55322)

Fixes the following regression introduced in v1.11
```julia
julia> using LinearAlgebra

julia> D = Diagonal(rand(4));

julia> T = Tridiagonal(Vector{BigFloat}(undef, 3), Vector{BigFloat}(undef, 4), Vector{BigFloat}(undef, 3))
4×4 Tridiagonal{BigFloat, Vector{BigFloat}}:
 #undef  #undef     ⋅       ⋅ 
 #undef  #undef  #undef     ⋅ 
    ⋅    #undef  #undef  #undef
    ⋅       ⋅    #undef  #undef

julia> copyto!(T, D)
ERROR: UndefRefError: access to undefined reference
Stacktrace:
  [1] getindex
    @ ./essentials.jl:907 [inlined]
  [2] _broadcast_getindex
    @ ./broadcast.jl:644 [inlined]
  [3] _getindex
    @ ./broadcast.jl:675 [inlined]
  [4] _broadcast_getindex
    @ ./broadcast.jl:650 [inlined]
  [5] getindex
    @ ./broadcast.jl:610 [inlined]
  [6] macro expansion
    @ ./broadcast.jl:973 [inlined]
  [7] macro expansion
    @ ./simdloop.jl:77 [inlined]
  [8] copyto!
    @ ./broadcast.jl:972 [inlined]
  [9] copyto!
    @ ./broadcast.jl:925 [inlined]
 [10] materialize!
    @ ./broadcast.jl:883 [inlined]
 [11] materialize!
    @ ./broadcast.jl:880 [inlined]
 [12] _copyto_banded!(T::Tridiagonal{BigFloat, Vector{BigFloat}}, D::Diagonal{Float64, Vector{Float64}})
    @ LinearAlgebra ~/.julia/juliaup/julia-nightly/share/julia/stdlib/v1.12/LinearAlgebra/src/special.jl:323
 [13] copyto!(dest::Tridiagonal{BigFloat, Vector{BigFloat}}, src::Diagonal{Float64, Vector{Float64}})
    @ LinearAlgebra ~/.julia/juliaup/julia-nightly/share/julia/stdlib/v1.12/LinearAlgebra/src/special.jl:315
 [14] top-level scope
    @ REPL[4]:1
```
After this PR
```julia
julia> copyto!(T, D)
4×4 Tridiagonal{BigFloat, Vector{BigFloat}}:
 0.909968  0.0        ⋅         ⋅ 
 0.0       0.193341  0.0        ⋅ 
  ⋅        0.0       0.194794  0.0
  ⋅         ⋅        0.0       0.506905
```
The current implementation used an optimization that may not be
applicable for non-isbits types, and this PR ensures that we always read
from the source and write to the destination.
@KristofferC
Copy link
Member

Seems to require #54041 to be relevant for backporting?

@jishnub
Copy link
Contributor Author

jishnub commented Aug 19, 2024

You're right, this does not need backporting.

@jishnub jishnub removed the backport 1.11 Change should be backported to release-1.11 label Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants