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

Bump MAX_TYPE_DEPTH #10230

Merged
merged 1 commit into from
Jul 30, 2015
Merged

Bump MAX_TYPE_DEPTH #10230

merged 1 commit into from
Jul 30, 2015

Conversation

mlubin
Copy link
Member

@mlubin mlubin commented Feb 17, 2015

Bumping MAX_TYPE_DEPTH enables efficient use of types parameterized by GenericGraph objects from the Graphs package. See JuliaAttic/OldGraphs.jl#171.

CC @JeffBezanson

@mlubin
Copy link
Member Author

mlubin commented Feb 18, 2015

@timholy, any idea why this breaks the subarray tests?

@JeffBezanson
Copy link
Member

This seems to be related to staged functions getting invoked on approximate types. There's a good chance we should just not do that. For one thing, we haven't yet taken steps to ensure monotonicity. For example, f(::Any) could give Int and f(::Array) could give String, which is unsound. It's also difficult and often useless to try to make a staged function work on approximate types.

I tested this by adding

    if (!jl_is_leaf_type(tt))
        jl_error("");

to jl_instantiate_staged.

@mlubin
Copy link
Member Author

mlubin commented Mar 5, 2015

IIRC @Keno had argued that this is useful. I also think it's fishy to invoke staged functions on approximate types.

@Keno
Copy link
Member

Keno commented Mar 5, 2015

Admittedly, I haven't yet found a specific use case for staged functions on approximate types. The basic area where I could see it being useful is to generate code for say an array where you know the number of dimensions, but not the type. Might also help with some of the concerns around staged functions in static compilation. I'm fine with disallowing this use case until somebody finds a concrete one though (easier going this way than the other).

@JeffBezanson
Copy link
Member

Unfortunately I already found a problem with this plan. Some functions do not use the types of some arguments. E.g. _mapreducedim! does not look at the type of its second argument (op). It also happens that we usually can't infer the type of this argument, so it gets passed as Any. Then disallowing calling the staged function with Any prevents us from inferring a type for the function.

@mbauman
Copy link
Member

mbauman commented Mar 5, 2015

Is this an example of the issue you're talking about, Jeff?

julia> stagedfunction f(A, idxs...)
         quote
           R = sub(A, idxs...)
           println(ndims(R), "-dim ", typeof(R))
           R
         end
       end
f (generic function with 1 method)

julia> A = reshape(1:32,2,2,2,2,2);

julia> f(A, 1:2, :, :, :, 1)
2-dim SubArray{Int64,2,Array{Int64,5},(UnitRange{Int64},Union(Colon,Int64)...),1}
2x2x2x2 SubArray{Int64,4,Array{Int64,5},(UnitRange{Int64},Colon,Colon,Colon,Int64),1}:
[:, :, 1, 1] =

@timholy
Copy link
Member

timholy commented Mar 5, 2015

#8501!

@timholy
Copy link
Member

timholy commented Mar 5, 2015

Sorry, I meant #8504. Keno didn't like #8973 before, but maybe it's OK now? It's a lot easier than #8504 (comment).

@JeffBezanson
Copy link
Member

The good news is that in this case the method cache seems ok; there are only concrete types in the cache for sub_unsafe. I think the only problem is the type unsoundness I mentioned above. Greatly simplified, sub_unsafe(I) returns SubArray{$I}(...), which is incorrect if I is an abstract type. I think for now we have to make this and other staged functions throw errors on abstract type arguments used in ways like this.

@timholy
Copy link
Member

timholy commented Mar 5, 2015

I'm fine with this plan. Jeff, I know how to make _mapreducedim! not be a stagedfunction anymore.

@mlubin: I'm sorry I didn't see this when you first asked it. Buried in the pile of julia-related email.

@JeffBezanson
Copy link
Member

Increasing the max depth now works, but seems to make the compiler slower (as measured by the test suite).

timholy referenced this pull request Mar 5, 2015
this is a bit drastic, but otherwise there is too much risk of type
unsoundness. fixes #8504
@JeffBezanson
Copy link
Member

@timholy great, it would be good to have fewer staged functions, used only when really necessary.

@mbauman
Copy link
Member

mbauman commented Mar 5, 2015

Is it actually making the compiler slower? Or is it simply compiling more specialized methods?

@JeffBezanson
Copy link
Member

Some individual tests were taking significantly longer. Later I noticed a few tests were slightly faster. Clearly some objectivity was needed, so I looked at the total time and overall the tests took 4% longer. So maybe not that big an effect after all.

@mlubin
Copy link
Member Author

mlubin commented Mar 9, 2015

Currently failing on 64-bit appveyor:

     * core                exception on 1: ERROR: LoadError: test failed: ((T,S) is Union())
 in expression: typeintersect((T,Ptr{T}),(Ptr{S},S)) === Bottom
 in error at error.jl:19
while loading core.jl, in expression starting on line 31
ERROR: LoadError: LoadError: test failed: ((T,S) is Union())
 in expression: typeintersect((T,Ptr{T}),(Ptr{S},S)) === Bottom
 in error at error.jl:19
while loading core.jl, in expression starting on line 31
while loading C:\projects\julia\test\runtests.jl, in expression starting on line 3

Is this unrelated?

@tkelman
Copy link
Contributor

tkelman commented Mar 10, 2015

Yes, unrelated and intermittent. 0576dad was intended to address that. I restarted the build here.

edit: now the failure is just #10045

@mlubin
Copy link
Member Author

mlubin commented Mar 11, 2015

Okay to merge then?

@mlubin
Copy link
Member Author

mlubin commented Apr 14, 2015

Bump. Rebased and passing now.

@mbauman
Copy link
Member

mbauman commented Jul 30, 2015

Bump? (ref #12289 (comment))

JeffBezanson added a commit that referenced this pull request Jul 30, 2015
@JeffBezanson JeffBezanson merged commit 592fc7e into JuliaLang:master Jul 30, 2015
@mlubin mlubin deleted the typedepth branch July 30, 2015 14:49
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.

6 participants