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

@inbounds isn't propagated for CartesianIndices #42115

Closed
Tokazama opened this issue Sep 3, 2021 · 2 comments · Fixed by #42119
Closed

@inbounds isn't propagated for CartesianIndices #42115

Tokazama opened this issue Sep 3, 2021 · 2 comments · Fixed by #42119

Comments

@Tokazama
Copy link
Contributor

Tokazama commented Sep 3, 2021

Recent discussion in ArrayInterface.jl demonstrated that @inbounds isn't fully propagated when using CartesianIndices to convert an integer to a CartesianIndex.
Examples across multiple versions and hardware are more fully demonstrated here JuliaArrays/ArrayInterface.jl#199).

I think the offending line is the missing @propagate_inbounds here:

function _getindex(::IndexCartesian, A::AbstractArray, I::Vararg{Int,M}) where M

This could probably be fixed here also:

function _getindex(::IndexLinear, A::AbstractArray, I::Vararg{Int,M}) where M

I think @timholy and @mbauman wrote the original code here.
Can we just put @propagate_inbounds in front of these?

@mbauman
Copy link
Sponsor Member

mbauman commented Sep 3, 2021

Those implementations look ok to me; @propagate_inbounds might possibly be needed on their caller function, but they will appropriately skip the boundscheck if called with @inbounds as written. Hard for me to quickly identify where the problem is in the linked issue. Note that @propagate_inbounds is only needed if you aren't using @inbounds in the implementation and want to rely upon the caller's inbounds state.

@Tokazama
Copy link
Contributor Author

Tokazama commented Sep 4, 2021

K. Trying with this inbounds_getindex(x, i) = @inbounds(x[i]) produces the following errors when something is out of bounds.

ERROR: BoundsError: attempt to access 2-element Base.OneTo{Int64} at index [3]
Stacktrace:
  [1] throw_boundserror(A::Base.OneTo{Int64}, I::Int64)
    @ Base ./abstractarray.jl:691
  [2] getindex
    @ ./range.jl:863 [inlined]
  [3] _broadcast_getindex_evalf
    @ ./broadcast.jl:670 [inlined]
  [4] _broadcast_getindex
    @ ./broadcast.jl:643 [inlined]
  [5] #29
    @ ./broadcast.jl:1119 [inlined]
  [6] ntuple
    @ ./ntuple.jl:49 [inlined]
  [7] copy
    @ ./broadcast.jl:1119 [inlined]
  [8] materialize
    @ ./broadcast.jl:904 [inlined]
  [9] getindex
    @ ./multidimensional.jl:357 [inlined]
 [10] inbounds_getindex(x::CartesianIndices{2, Tuple{Base.OneTo{Int64}, Base.OneTo{Int64}}}, i1::Int64, i2::Int64)
    @ Main ./REPL[8]:1
 [11] top-level scope
    @ REPL[10]:1

This doesn't happen with --check-bounds=no.

I'm assuming that once we get into the broadcasting machinery there's a big risk of inadvertently messing things up, so could we just avoid broadcasting here:

CartesianIndex(getindex.(iter.indices, I))

and instead just call CartesianIndex(I)

This issue was closed.
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 a pull request may close this issue.

2 participants