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

flatten the call chain of checkbounds a bit #17340

Merged
merged 1 commit into from
Jul 9, 2016
Merged

flatten the call chain of checkbounds a bit #17340

merged 1 commit into from
Jul 9, 2016

Conversation

JeffBezanson
Copy link
Sponsor Member

checkbounds is surprisingly complicated. The call chain consists of checkbounds -> checkbounds(Bool) -> _chkbounds -> checkindex and linearindices. This is especially an issue for ranges, where the bounds check code looks like:

        $(Expr(:boundscheck, true))
        SSAValue(6) = (Core.tuple)(i)::Tuple{Int64}
        $(Expr(:inbounds, false))
        # meta: location abstractarray.jl checkbounds 222
        # meta: location abstractarray.jl checkbounds 194
        SSAValue(8) = (Core.getfield)(SSAValue(6),1)::Int64
        # meta: location abstractarray.jl _chkbounds 197
        SSAValue(9) = $(Expr(:invoke, indices1(::LinSpace{Float64}), :(Base.indices1), SSAValue(5)))
        # meta: pop location
        # meta: pop location
        SSAValue(7) = (Base.box)(Base.Bool,(Base.and_int)((Base.sle_int)(1,SSAValue(8))::Bool,(Base.sle_int)(SSAValue(8),(Core.getfield)(SSAValue(9),:stop)::Int64)::Bool))
        unless SSAValue(7) goto 18
        #temp#@_4 = SSAValue(7)
        goto 20
        18: 
        #temp#@_4 = $(Expr(:invoke, throw_boundserror(::LinSpace{Float64}, ::Tuple{Int64}), :(Base.throw_boundserror), SSAValue(5), SSAValue(6)))
        20: 
        # meta: pop location
        $(Expr(:inbounds, :pop))
        #temp#@_4
        $(Expr(:boundscheck, :pop))

Yikes. This change simplifies it a bit, especially for Vector and Range, where the call to indices can easily be avoided.

@JeffBezanson JeffBezanson mentioned this pull request Jul 8, 2016
4 tasks
@JeffBezanson JeffBezanson merged commit dad6836 into master Jul 9, 2016
@tkelman tkelman deleted the jb/checkb branch July 9, 2016 02:25
end
function checkbounds{T}(::Type{Bool}, A::Union{Array{T,1},Range{T}}, i::Integer)
@_inline_meta
(1 <= i) & (i <= length(A))
Copy link
Contributor

@tkelman tkelman Jul 9, 2016

Choose a reason for hiding this comment

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

should this be using first and last for ranges?

@timholy
Copy link
Sponsor Member

timholy commented Jul 9, 2016

I am surprised that indices1 didn't inline. Note that if you force enough inlining (on both indices1 and linearindices), the previous call chain reduces to

julia> a = linspace(1,5,3)
3-element LinSpace{Float64}:
 1.0,3.0,5.0

julia> @code_typed checkbounds(a, 2)
1-element Array{Any,1}:
 LambdaInfo for checkbounds(::LinSpace{Float64}, ::Int64)
:(begin  # line 226:
        # meta: location abstractarray.jl checkbounds 197
        SSAValue(1) = (Core.getfield)(I,1)::Int64
        # meta: location abstractarray.jl _chkbounds 200
        # meta: location abstractarray.jl linearindices 67
        # meta: location abstractarray.jl indices1 52
        # meta: location abstractarray.jl indices 44
        #30 = $(Expr(:new, :(Base.##30#31)))
        SSAValue(2) = #30
        # meta: location range.jl size 316
        # meta: location range.jl length 336
        SSAValue(4) = (Core.getfield)(A,:len)::Float64
        # meta: pop location
        # meta: pop location
        SSAValue(6) = (Base.box)(Base.Int,(Base.checked_fptosi)(Base.Int,(Base.select_value)((Base.slt_int)((Base.box)(Int64,(Base.box)(Base.Float64,(Base.sub_float)((Core.getfield)(A,:len)::Float64,(Base.box)(Float64,(Base.sitofp)(Float64,1))))),0)::Bool,(Base.box)(Base.Float64,(Base.add_float)((Base.box)(Float64,(Base.sitofp)(Float64,1)),SSAValue(4))),SSAValue(4))::Float64))
        # meta: location tuple.jl map 85
        SSAValue(5) = SSAValue(6)
        # meta: pop location
        # meta: pop location
        # meta: pop location
        # meta: pop location
        # meta: pop location
        # meta: pop location
        SSAValue(0) = (Base.box)(Base.Bool,(Base.and_int)((Base.sle_int)(1,SSAValue(1))::Bool,(Base.sle_int)(SSAValue(1),(Base.select_value)((Base.slt_int)(SSAValue(5),0)::Bool,0,SSAValue(5))::Int64)::Bool))
        unless SSAValue(0) goto 27
        return SSAValue(0)
        27: 
        return $(Expr(:invoke, throw_boundserror(::LinSpace{Float64}, ::Tuple{Int64}), :(Base.throw_boundserror), :(A), :(I)))
    end::Bool)

which should be fine.

The main reason for the indirection to _chkbounds was explained in the comment above those lines: like our getindex chain since #10525, a main design principle was to avoid causing unnecessary ambiguities with package code that wants to specialize checkbounds. This PR introduced some methods that specialize on the array type, and other methods that specialize on the index type. At least historically, separate specializations for two different arguments has been a formula for forcing package authors to create a whole bunch of definitions whose only purpose is to short-circuit ambiguities. Here a concrete example will probably be Interpolations, which wants to implement checkbounds for more specific array types but a more general index type (Real vs Integer). checkbounds could certainly have been designed more cleanly, but even from a theoretical standpoint, avoiding ambiguities when you want to be able to independently specialize two arguments appears to require an extra layer of indirection somewhere.

That said, ambiguities are not the problem they used to be, and the API has gotten general enough that there should rarely be a need to specialize it. So in contrast to getindex, I suspect that ambiguities with checkbounds will be rare. So I think this is OK.

timholy added a commit that referenced this pull request Jul 9, 2016
timholy added a commit that referenced this pull request Jul 9, 2016
timholy added a commit that referenced this pull request Jul 10, 2016
timholy added a commit that referenced this pull request Jul 11, 2016
timholy added a commit that referenced this pull request Jul 12, 2016
PallHaraldsson pushed a commit to PallHaraldsson/julia that referenced this pull request Aug 15, 2016
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
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.

None yet

3 participants