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

inline inexact method matches, repeated #7075

Merged
merged 13 commits into from
Jun 10, 2014
Merged

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Jun 2, 2014

continuation of #6677. that one got closed because I was merging into the printf2 branch, and it merged into master

this inserts a MethodError based on the argument types of the only matching function, then inlines the function, or it's call site, for the exact method match

@vtjnash vtjnash changed the title inline inexact method matches, repeat inline inexact method matches, repeated Jun 2, 2014
@JeffBezanson
Copy link
Sponsor Member

This is good but at this point I'm more concerned about the inlining-related performance regressions.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jun 2, 2014

The inexact match part of this is disabled amyways, due to a bug in method dispatch, so it just has some additional, unused infrastructure that can be used for testing ideas, and then also corrections to effect_free and tweaks to inline_worthy to directly address the performance issues.

@StefanKarpinski
Copy link
Sponsor Member

disabled amyways, due to a bug in method dispatch

additional, unused infrastructure that can be used for testing ideas

Both of these things sound super ominous.

@@ -2044,14 +2049,26 @@ function inlineable(f, e::Expr, atypes, sv, enclosing_ast)
sp = tuple(sp..., linfo.sparams...)
spvals = { sp[i] for i in 2:2:length(sp) }
for i=1:length(spvals)
if isa(spvals[i],TypeVar)
if contains_typevar(spvals[i])
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Are you sure this is necessary? A static parameter might equal, for example, Array.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

It shouldn't be necessary -- I was trying to work around #7074, where type inference decides to mutate the static parameter so that it can incorrectly call a blas function requiring Array{Complex{Float64}} with input of Array{Complex}

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Ok; looking at #7074 now.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Fixed; see if you can remove the workaround.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

i'm still running into a different issue:

julia> (String, Int64, Any) <: ((Type{TypeVar(:T,Any)}...,),)
false

which is resulting from the following answer from _methods

julia> Base._methods(length, (Type{TypeVar(:T,Base.Top)},), 1)
1-element Array{Any,1}:
 (((Type{T<:Top}...,),),(),length(t::(Any...,)) at tuple.jl:3)

which is determined while computing type inference for length(argtypes) in abstract_call_gf

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I don't get it --- the left side is a 3-tuple and the right side is a 1-tuple. Also String <: Type should of course be false.

Tuples are the only types for which length is applicable, so that looks ok to me.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

whoops, you're right. i simplified the example wrong:

julia> isa((TypeVar(:T,String), Int64, Any), (Type{TypeVar(:T,Any)}...,))
false

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Hmmm, the TypeVar(:T,String) should not be in there --- as you can see, it's not a type. Any sense of where it came from?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

not really. i can push my WIP if you want to play around. lookup up the call-stack a bit, you can see sort of see where it starts to gets introduced

(lldb) print jl_(f->linfo)
Base.abstract_call(Type{Void<:T<:Union(Any, Undef)}, Array{Any, 1}, Type{Void<:T<:Union(Any, Undef)}, Base.ObjectIdDict, Base.StaticVarInfo, Expr)
(lldb) print jl_(args[0])
Base.SubString{Void<:T<:String}
(lldb) print jl_(args[1])
Array{Any, 1}[:str, :i, Expr(:call, :prevind, :str, :j)::Any]
(lldb) print jl_(args[2])
(Void<:T<:String, Int64, Any)
(lldb) print jl_(args[3])
Base.ObjectIdDict(ht=Array{Any, 1}[#<null>, #<null>, :r, Any, :#s465, Undef, #<null>, #<null>, :#s458, Bool, #<null>, #<null>, #<null>, #<null>, :#s464, Undef, #<null>, #<null>, :limit, Int64, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, :i, Int64, :k, Any, #<null>, #<null>, #<null>, #<null>, :splitter, Char, :#s461, Bool, #<null>, #<null>, :n, Any, #<null>, #<null>, :#s467, Any, #<null>, #<null>, #<null>, #<null>, :keep_empty, Bool, :j, Any, :strs, Void<:_<:Array{Void<:_<:Base.SubString{Void<:T<:String}, 1}, :#s454, Undef, #<null>, #<null>, :#s466, Any, :str, Void<:T<:String])
(lldb) print jl_(args[4])
Base.StaticVarInfo(sp=(Void<:U<:Array{Void<:T<:Any, Void<:N<:Any}, Void<:_<:Array{Void<:_<:Base.SubString{Void<:T<:String}, 1}, Void<:T<:String, Void<:T<:String), cenv=Base.ObjectIdDict(ht=Array{Any, 1}[#<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>]), vars=Array{Any, 1}[:str, :splitter, :limit, :keep_empty, :strs, :i, :n, :r, :#s467, :#s466, :j, :k, :#s454, :#s461, :#s458, :#s465, :#s464], label_counter=15)
(lldb) print jl_(args[5])
Expr(:call, :SubString, :str, :i, Expr(:call, :prevind, :str, :j)::Any)::Any

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

note that the signature of the function abstract_call_gf in which we see the confusion occur is:

(lldb) print jl_(f->linfo)
Base.abstract_call_gf(Type{Void<:T<:Union(Any, Undef)}, Array{Any, 1}, Type{Void<:T<:Union(Any, Undef)}, Expr)
(lldb) print jl_(args[0])
Base.SubString{Void<:T<:String}
(lldb) print jl_(args[1])
Array{Any, 1}[:str, :i, Expr(:call, :prevind, :str, :j)::Any]
(lldb) print jl_(args[2])
(Void<:T<:String, Int64, Any)
(lldb) print jl_(args[3])
Expr(:call, :SubString, :str, :i, Expr(:call, :prevind, :str, :j)::Any)::Any

@JeffBezanson
Copy link
Sponsor Member

Is the problem only appearing with inline_incompletematch_allowed = true ?
If so, we can just keep it false. After all, this PR has two different kinds of changes, which I just love.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jun 4, 2014

sure, but that doesn't change the fact that julia dispatched to the wrong method:

julia> f(::Type) = 1
f (generic function with 1 method)

julia> f((TypeVar(:T,String), Int64, Any))
1

@JeffBezanson
Copy link
Sponsor Member

Well, a typevar isn't a type but a tuple with a typevar inside it is --- typevars can be type parameters.

In any case, you have to separate the three issues here: general inlining fixes, the partial match feature, and this dispatch bug if there is one. The bug might block one of those other things, but it will be easier to talk about and triage.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jun 5, 2014

it is hard to keep this straight, but I think this is the core of the issue:

julia> Base._methods(g, (Type{TypeVar(:T,Base.Top)},), 2)
1-element Array{Any,1}:
 (((Type{T<:Top}...,),),(),g(::(Type{T<:Top}...,)) at none:1)

whereas, the answer should have been:
(((Union(TypeVar(:T,Top),Type{TypeVar(:T,Top)})...,),),(),g(::(Type{T<:Top}...,)) at none:1)


or, alternatively formulated:

julia> Base.typeintersect(Type,Tuple)
(Type{T<:Top}...,)

should have returned:
(Union(Type{T<:Top}, T<:Top)...,)
because a Tuple of TypeVars is also a Type:

julia> isa((TypeVar(:T,Any),), Type)
true

@JeffBezanson
Copy link
Sponsor Member

Maybe we should just make TypeVar a kind of Type.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jun 6, 2014

I don't know. However, I'm guessing that Symbols and Ints may share the same issues. I guess there should be a distinction between Type and TypeParam = Union(Type, TypeVar, Int, Symbol)?

@JeffBezanson
Copy link
Sponsor Member

The problem really has to do with tuples, since each tuple may or may not be a type. A tuple of symbols or ints is never a type. However, a tuple of typevars is a type, despite neither tuples nor typevars being types by themselves. Making typevars types would fix this.

@JeffBezanson
Copy link
Sponsor Member

I think we should set inline_incompletematch_allowed = false and merge this.

@JeffBezanson
Copy link
Sponsor Member

update: with this change, #6681 is fixed, #6981 is fixed, but #5011 gets 2x worse.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jun 9, 2014

#5011 is a bit of a pathological case. we actually really don't want to be inlining that function, but we are having codegen issues with the alternative. the solution there is to do something more intelligent with the calling convention for variadic calls of leaf-types, such that the values can be passed in an unboxed fashion (or boxed on the stack).

@JeffBezanson
Copy link
Sponsor Member

I agree #5011 is a different beast.

JeffBezanson added a commit that referenced this pull request Jun 10, 2014
inline inexact method matches, repeated
@JeffBezanson JeffBezanson merged commit 78fdda4 into master Jun 10, 2014
@Keno
Copy link
Member

Keno commented Jun 10, 2014

whoa Jeff merged something ;)

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jun 10, 2014

that's probably somehow my fault.

(this did help to get a few bugs found and fixed along the way)

@vtjnash vtjnash deleted the jn/inline_partialmatch branch August 11, 2014 22:26
@simonster simonster mentioned this pull request Nov 23, 2014
vtjnash added a commit that referenced this pull request Apr 26, 2015
mbauman pushed a commit to mbauman/julia that referenced this pull request Jun 6, 2015
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

4 participants