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

setindex! no longer inlined for rank N>6 #9622

Closed
Jutho opened this issue Jan 5, 2015 · 9 comments
Closed

setindex! no longer inlined for rank N>6 #9622

Jutho opened this issue Jan 5, 2015 · 9 comments
Labels
performance Must go faster

Comments

@Jutho
Copy link
Contributor

Jutho commented Jan 5, 2015

I just noticed this today on latest master (Version 0.4.0-dev+2498), I guess this must be regression?

f6(A)=(A[1,1,1,1,1,1]=1.)
f7(A)=(A[1,1,1,1,1,1,1]=1.)
code_typed(f6,(Array{Float64,6},))
code_typed(f7,(Array{Float64,7},))

returns

1-element Array{Any,1}:
 :($(Expr(:lambda, Any[:A], Any[Any[],Any[Any[:A,Array{Float64,6},0]],Any[]], :(begin  # none, line 1:
        (top(arrayset))(A::Array{Float64,6},1.0,1,1,1,1,1,1)::Array{Float64,6}
        return 1.0
    end::Float64))))

versus

1-element Array{Any,1}:
 :($(Expr(:lambda, Any[:A], Any[Any[],Any[Any[:A,Array{Float64,7},0]],Any[]], :(begin  # none, line 1:
        setindex!(A::Array{Float64,7},1.0,1,1,1,1,1,1,1)::Any
        return 1.0
    end::Float64))))

Consequently, indexing into an array of rank>6 becomes slow. Indexing with a CartesianIndex could be fixed by letting the stagedfunction generate the arrayset call automatically, but I guess this also needs to be fixed for normal indexing?

@Jutho
Copy link
Contributor Author

Jutho commented Jan 5, 2015

For getindex, inlining stops for N>7.

@timholy
Copy link
Member

timholy commented Jan 5, 2015

Dupe of #5393. Jeff closed that, but I'm not sure it's redundant with anything else, so perhaps we should leave this open.

As @simonster said there,

I think performance drops because MAX_TUPLETYPE_LEN in inference.jl is 8, which appears to prevent getindex from getting inlined when it is passed >8 arguments.

@Jutho
Copy link
Contributor Author

Jutho commented Jan 5, 2015

I thought I had seen something like this before, but couldn't find it. Thanks. Ok then, I hope you agree that we should at least change indexing with CartesianIndex to immediately return the arrayref and arrayset instruction, so as to have at least one way of fast indexing?

@Jutho
Copy link
Contributor Author

Jutho commented Jan 5, 2015

Although with the @inline macro and/or stagedfunctions it should also be possible to make getindex and setindex! fast, no?

@timholy
Copy link
Member

timholy commented Jan 5, 2015

I certainly support doing something (whatever seems best) to make this fast.

@mbauman
Copy link
Member

mbauman commented Jun 12, 2015

Hm, this got worse after #10525. Inlining now stops for rank N>4. I'll try to look into this one a bit more.

@mbauman
Copy link
Member

mbauman commented Jun 20, 2015

Ah, found it. We're still one-worse than we were since I dispatch to an internal function that has one more argument (to specify the LinearIndexing trait), and so we hit MAX_TUPLETYPE_LEN one dimension sooner.

mbauman added a commit that referenced this issue Jun 20, 2015
Helps #9622; getindex is now inlined up to N=6 and setindex! up to N=5. Any higher dimensions are limited by MAX_TUPLETYPE_LEN.
@matthieugomez
Copy link
Contributor

I encountered this issue. If it's hard to solve now, a warning would be nice - it was hard to pinpoint the source of the problem in my code.

@KristofferC
Copy link
Member

This seems fixed now,

julia> code_typed(f10,(Array{Float64,10},))
1-element Array{Any,1}:
 CodeInfo(:(begin 
        (Base.arrayset)(A,(Core.typeassert)(1.0,Float64)::Float64,1,1,1,1,1,1,1,1,1,1)::Array{Float64,10}
        return 1.0
    end))=>Float64

Please repoen if this is still a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

No branches or pull requests

6 participants