call overloading, and make constructors use it #8712

Merged
merged 53 commits into from Oct 23, 2014

Projects

None yet
@JeffBezanson
Member

Behold, after a week of red Xes, the tests are passing.

This replaces #8008 and also redesigns constructors to use call overloading instead of having some types magically also be generic functions.

An inner constructor looks like

call{T}(::Type{MyType{T}}, args...) = ...

and an outer constructor looks like

call{T}(::Type{MyType}, x::T) = MyType{T}(x)

Now you can also have things that are in-between, for example this SubArray constructor:

    function call{T,A<:Array,I<:(Any,)}(::Type{SubArray{T,0,A,I}}, p::A, i::(Int,))
        new{T,0,A,I}(p, i, (), Int[], i[1])
    end

Notice new{...} has been added and is needed in cases like this.

Of course, you can also define constructors for abstract types which should be helpful in several cases that have come up over time.

An interesting side effect of this change is that this:

convert{T}(::Type{T}, x::T) = x

now correctly implements "conversions" to supertypes (e.g. convert(Number, 1)).


This raises several interesting discussion questions that have not been resolved yet:

  • How should method reflection work? Do we preserve the illusion that types are "callable"?
  • What to do with the Callable type? Maybe make Function an abstract type instead?
  • How do we make call fall back to convert, and also to call in lower-level modules (see #8008 (comment))?
  • We can now remove the dreaded "lower-case" conversion functions, but should Int8(x) be vectorized?
@stevengj
Member

@JeffBezanson, this line gives

ERROR: type: issue2403func: in apply, expected Function, got Issue2403
 in issue2403func at none:1

but the other tests work. What am I missing?

(On the phone, you mentioned something about having to change type inference somewhere, but I'm not quite sure what you were referring to. Something in inference.jl?)

@stevengj
Member

Another tricky case:

julia> type Foo
       x
       Base.convert(::Type{Foo}, x) = new(int(x))
       end

julia> convert(Foo, 7)
Foo(7)

julia> Foo(3.2)
ERROR: `Foo` has no method matching Foo(::Float64)

julia> call(Foo, 3.2)
Foo(3)

julia> methods(Foo)
#0 methods for generic function "Foo":

The cleanest way to resolve this might be to get rid of constructors as "generic functions" per se, and just make them methods added to call. That way there is no question whether you want to dispatch on call or on Foo methods.

Member

Yes, that's what I was saying yesterday --- constructors should be dispatched on types by call the way convert is. This is internally simpler and more general as well.

Member

Here's a weird behavior:

julia> Base.call(x::Int, y::Int) = x + 3y
call (generic function with 3 methods)

julia> let x=10
       x(3) == 19
       end
true

julia> let x=10
       Base.Test.@test x(3) == 19
       end
ERROR: test error during x(3) == 19
type: anonymous: in apply, expected Function, got Int64
 in anonymous at test.jl:62
 in do_test at test.jl:37
 in anonymous at no file:2

@JeffBezanson, how is macro expansion of @test triggering a different code path here?

Update: seems to be a problem with globals in functions?

julia> Base.call(x::Int, y::Int) = x + 3y
call (generic function with 3 methods)

julia> x = 3
3

julia> x(7)
24

julia> g = () -> x(3)
(anonymous function)

julia> g()
ERROR: type: anonymous: in apply, expected Function, got Int64
 in anonymous at none:1

julia> function h()
       x(3)
       end
h (generic function with 1 method)

julia> h()
ERROR: type: h: in apply, expected Function, got Int64
 in h at none:2
Member

Hmm, I've just added support for switching over to call from jl_f_apply and jl_f_kwcall, but it's still not working, so I must be missing another place. I still haven't changed applicable and invoke, but I don't think it could be one of those because the error says in apply.

Member

Okay, the problem is that emit_func_check needs to go away...

I tried to do this with 762a727, but it broke something else.

@stevengj
Member

Not sure what is going on here. After this patch, make clean && make gives

int.jl
error during bootstrap: LoadError(at "sysimg.jl" line 42: LoadError(at "int.jl" line 280: UndefVarError(var=:start)))

@JeffBezanson or @vtjnash, any tips would be welcome.

Member

The --args, ++nargs seems suspicious, but I can't fully review that from a phone

Member

The rest of the code only accesses args[1] or later, so this just shifts the pointer to make args[0] the first argument.

Also, that part of the code seems to be working...it was used in the previous commit to implement call overloading, and worked fine until this patch.

@stevengj
Member

This theFunc = emit_expr(...) can probably be moved into the else clause, since presumably theFunc->getType() != jl_pvalue_llvmt will always be false if hdtype==(jl_value_t*)jl_function_type?

@vtjnash
Member

the issue is probably that this depends too heavily on type-inference, so it tries to turn everything into a call(f,...), until inference gets a chance to start running. might be necessary. i think this means you will need to always emit the emit_func_check branch here, just changing the else clause. it's probably better to be avoiding the tuple pack/unpack anyways.

Member

after thinking about this some more, i'm not sure if you want this here at all. perhaps, the branch should be handled by jl_apply_generic? in either case, it needs a custom lowering pass in inlining_pass, similar to ^ and apply

Member

I can understand why this might not be optimal, but why would turning lots of things into call(f, ...) cause an undefined-variable error? ... oh, I see, the tuple unpacking.

Member

@vtjnash at the point where it is failing in building the sysimg (Int.jl), shouldn't the tuple start method already be defined (it preceeds int.jl in build order)? I did some debugging after you left this afternoon @stevengj (replacating pretty much what @vtjnash posted) but saw that it was failing in eval with start undefined, and to me this didn't make any sense as the start method exists for tuples at that point (or it doesn't and I'm just misunderstanding how the sysimg build process works).

Member

That's a good point. In fact, start is never defined, and thus ... can't be used inside of Core, ever. (well, without first defining Core.start)

julia> Core.start
ERROR: start not defined
Member

oh, I see, the tuple unpacking.

i see my comments aren't very well connected. this one was supposed to be first:
#8008 (comment)

JeffBezanson added some commits Sep 1, 2014
@JeffBezanson JeffBezanson get basic call overloading working with 1 bootstrap stage b430c48
@JeffBezanson JeffBezanson Merge branch 'master' into call_overload
Conflicts:
	src/codegen.cpp
	test/core.jl
27e476c
@JeffBezanson JeffBezanson update emit_call for jl_fptr_llvmt => jl_pfptr_llvmt change bd955f2
@JeffBezanson JeffBezanson Merge branch 'master' of github.com:JuliaLang/julia into call_overload
Conflicts:
	base/base.jl
	test/core.jl
a4dcc69
@JeffBezanson JeffBezanson some work on call overloading
- look up call in the right module instead of using jl_call_func
- some inference support for call overloading
- pass `call` to _apply and kwcall so they use the containing module's definition
- deprecate explicit apply(), rename the builtin to _apply
aeae50f
@JeffBezanson JeffBezanson Merge branch 'master' of github.com:JuliaLang/julia into call_overload
Conflicts:
	base/deprecated.jl
	base/inference.jl
06471db
@JeffBezanson JeffBezanson fix codegen for changes to apply() for call overloading 2463704
@JeffBezanson JeffBezanson temporarily restore inference/startup performance 5b03766
@JeffBezanson JeffBezanson fully redesign constructors to use `call` overloading
this does not work yet, but gets past the first bootstrap stage.

so far this is a significant net simplification.
51a3069
@StefanKarpinski

Yikes. That's a typo fix that should be ported to master and release-0.3. cc: @jiahao, @andreasnoack

Member

Already on master.

@StefanKarpinski
Member

I'm disturbed by how much code there seems to be in linalg that appears to be entirely untested.

Member

This was our attempt to mimic triangular dispatch. A lot of the code for special matrix types needs to reason about the type of the container and the type of the matrix element. The signature doesn't make the compiler complain (yet), but it doesn't quite work as expected. @andreasnoack has encountered issues where the type parameter T of the container would silently promote (e.g. from Matrix{Float64} to Matrix{BigFloat}), but the declared eltype of the container (S) doesn't change, and so the kernels had to be rewritten to manually update S at runtime.

This is a problem that needs to be thought about more carefully. Ref: #8240

Member

While the syntax is still a little ugly for the moment, you can get triangular dispatch with the trick in #2345. I've had to do a lot of that with Images ever since the transition to incorporating Color.

Member

The behavior that falls out of an implementation detail of the compiler here is that the T in AbstractMatrix{T} basically has no effect, so on master I don't think this commit changes any functionality. However on my jb/call_constructors branch the implementation changed, and this gives a "T not defined" error, which is why I did this.

Member

@JeffBezanson Thanks for the explanation. Ironically, as @jiahao mentioned, I encountered the problem this Friday without realising the root cause. I have been living in delusion. Probably, I'll have to write inner constructors for all the types to ensure that T actually carries information about the element type.

@StefanKarpinski There is room for improvement, but I think that the coverage of exported functions in LinAlg is decent. However, promotion is a challenge. We try, but it is difficult to cover all corners of possible promotions. Regarding this commit, the wrong assumption that T had an effect in AbstractMatrix{T} has probably not been discovered before because the problematic constructors are rarely used, e.g. when constructing a triangular matrix it is more common to write Triangular(A, :L) than Triangular{Float64,SubArray{Float64,2,Array{Float64,2},(UnitRange{Int64},UnitRange{Int64})},:L,false}(A).

Member

This is less bad than I thought it was since it used to accidentally work – I thought it was fundamentally broken and that brokenness simply hadn't been discovered. There's been a fair amount of that sort of thing in the linalg code in the past but it does seem to be much better since you guys have been putting so much work into it.

JeffBezanson added some commits Oct 13, 2014
@JeffBezanson JeffBezanson fix a memory bug in codegen of getfield on a 0-size struct 208b2cd
@JeffBezanson JeffBezanson type system improvements needed for constructors-via-call
- make typeintersect((Rational{T},T),(Rational{Integer},Int)) == (Rational{Integer},Int)
  This used to be Bottom, since the T's had to match exactly. Now if T
  appears in covariant position, subtypes can also match. This is a
  good change anyway, but turns out to be necessary for the new constructor
  design. We have a constructor `Rational{T}(x::T, y::T)` which now gets
  lowered to `call{T}(::Type{Rational{T}}, x::T, y::T)`, so obviously we
  must allow x and y to be any subtypes of T.
  This also allows convert_default to be replaced with
  `convert{T}(::Type{T}, x::T) = x` (to be done next).

- making that work required an improved constraint solving algorithm in
  type intersection. the new algorithm should be much more robust, but
  it yields more typevars in its answers, for example
  `typeintersect((T,AbstractArray{T}),(Any,Array{Number,1}))` gives
  `(_<:Number,Array{Number,1})`.

  Hopefully this will not cause problems. But I can imagine doing a
  post-processing step to replace `_<:T` in covariant position with
  just `T`. In the meantime, to further legitimize such results I
  also made the next change:

- make TypeVar a subtype of Type
7068390
@JeffBezanson JeffBezanson a couple tweaks to type intersection that were helpful when eliminati…
…ng convert_default
d5aebc7
@JeffBezanson JeffBezanson remove convert_default builtin c62c8f0
@JeffBezanson JeffBezanson code cleanup, fix error displaying types, change Callable 4e0dd59
@jakebolewski

Why is this change needed?

Member

Presumably to avoid excess specialization of the CallStack constructor.

Member

Yes - that particular slot sees basically every potential call signature in the system.

@JeffBezanson JeffBezanson added this to the 0.4 milestone Oct 17, 2014
@JeffBezanson JeffBezanson referenced this pull request Oct 17, 2014
Closed

WIP: Call overload #8008

@StefanKarpinski
Member

How should method reflection work? Do we preserve the illusion that types are "callable"?

If you think of a method f(a::A,b::B) as having signature (f,A,B) then methods(f) is just conceptually going through all the candidate methods in the system and returning the ones that have head f. So it seems to me that if T(a::A,b::B) will work, then it would be good to return it. The issue is that you need some way of knowing all the call methods that might apply and querying which things they know how to do.

@stevengj
Member

@StefanKarpinski, querying doesn't seem hard to me. If you call methods(x) and x is any object that is not a Function, then it can call methods(call) and run through the list to find which call methods apply to x as the first argument.

The only question seems to be how the resulting list should be displayed in the case where x is a type. My first inclination would be to display them as x(args...), but this may not be very robust. As in Jeff's SubArray example above, there may be constructors whose method signatures aren't easily expressible as "type functions".

Of course, we could just print methods(T) as a sequence of call(::Type{T}, ...) functions for now, and fix it later if people complain. How show(::Method) is implemented does not seem to raise a lot of compatibility questions.

@JeffBezanson
Member

I'm actually not so sure anymore that we want call(T::Type, args...) = convert(T, args...). It means a sequence like this works for all types:

x = SomeRandomThing()
...
SomeRandomThing(x)   # probably a no-op?

There could certainly be types for which this is misleading, as it appears to mandate a kind of "copy constructor" that might not make sense.

We do want this behavior for T<:Number, but I'm not sure if there's any more general rule. It's best for type inference to limit the applicability of definitions as much as possible; ideally you want each type to fully control how it can be constructed.

JeffBezanson added some commits Oct 18, 2014
@JeffBezanson JeffBezanson remove error for failing to import a non-function used in a method de…
…finition, since the method will actually be added to `call`
a3d637f
@JeffBezanson JeffBezanson more surgical approach to importing Core.call
this is not ideal but will give us more flexibility for now
12ecc25
@toivoh
Member
toivoh commented Oct 19, 2014

I thought we already had counterexamples against the convert fallback, such as Frame(Frame())

@stevengj
Member

@toivoh, the original argument was that the counter-examples would be rare; in those cases you can easily override the constructor. In contrast, in most cases it is extremely convenient and expected that Foo(x) falls back to convert (it is usually weird to be able to do convert(Foo, x) but not Foo(x)), and not having this as a default means that almost every type will want to include this fallback itself.

@JeffBezanson, I don't understand the issue with including a Foo(x::Foo) = x constructor by default; what practical difficulty does this raise?

@nalimilan
Contributor

It would be pretty confusing to have Foo(x::Foo) = x for most types, but Frame(Frame()) pack a frame into another frame (if I understand correctly @toivoh's point).

@toivoh
Member
toivoh commented Oct 19, 2014

Sorry @stevengj, now I seem to remember.

@JeffBezanson
Member

Yeah, most likely the only defensible options are providing the Foo(x::Foo) = x default everywhere or nowhere.

I think it just annoys me when the system imposes something on you that cannot be gotten rid of. All you can do is define Foo(x) to throw an error, but you can't eliminate the extra applicable method.

JeffBezanson added some commits Oct 19, 2014
@JeffBezanson JeffBezanson fix order of initialization in jl_new_datatype (memory bug) d382a46
@JeffBezanson JeffBezanson fix a missing GC root in codegen 5b9ec88
@JeffBezanson JeffBezanson fix crash in sortrows
the subtype check in gf.c:ml_matches (line 1834) was causing an extra
matching method to be missed, when we have both call(::Type{Perm},...)
and call(::Type{Perm{O,V}},...)
d2dd9f1
@JeffBezanson JeffBezanson slightly cut down lowered code for kwcall 4fdf200
@JeffBezanson JeffBezanson Merge branch 'master' of github.com:JuliaLang/julia into jb/call_cons…
…tructors

Conflicts:
	src/julia-syntax.scm
6236a24
@stevengj
Member

Along the lines of #8710, maybe Foo(x) should default to convert(Foo, x)::Foo rather than just convert(Foo, x).

@JeffBezanson
Member

Ready to merge. Any final comments?

@StefanKarpinski
Member

"Yay"

@timholy
Member
timholy commented Oct 22, 2014

Haven't had time to follow this at all, but superficially it seems like a pretty exciting development.

@jakebolewski
Member

This is exciting.

One comment, I'm seeing a 25% regression in performance in a parser test I have (compared to master). Do you have any idea about potential performance regressions with this branch?

@JeffBezanson
Member

One general source of regressions is constructor calls where we can't infer the argument types exactly. The extra applicable call method can kill type inference there.

There was another regression (now fixed) in the perf suite due to the wrong method getting selected because of one of my type system tweaks. It's not impossible there's another thing like that.

@JeffBezanson
Member

Comparing profile output should be helpful for this.

@tkelman tkelman commented on the diff Oct 22, 2014
src/codegen.cpp
+ if (result != NULL) return result;
+ }
+
+ hdtype = expr_type(a0, ctx);
+ definitely_function |= (hdtype == (jl_value_t*)jl_function_type);
+ definitely_not_function |= (jl_is_leaf_type(hdtype) && !definitely_function);
+
+ assert(!(definitely_function && definitely_not_function));
+
+ int last_depth = ctx->argDepth;
+ Value *result;
+
+ if (definitely_not_function) {
+ if (f == NULL) {
+ f = jl_module_call_func(ctx->module);
+ Value *r = emit_known_call((jl_value_t*)f, args-1, nargs+1, ctx, &theFptr, &f, expr);
@tkelman
tkelman Oct 22, 2014 Member

strange that this gives an unused variable warning on linux but not osx?

@StefanKarpinski
Member

How about merging and then working on the performance regression after? That may make it easier for others like @vtjnash to help.

@tkelman
Member
tkelman commented Oct 22, 2014

There's some bad interaction here with sys.dll on Windows. If I do usr/bin/julia --check-bounds=yes test/runtests.jl linalg1, it segfaults if sys.dll is present, works fine without. julia-debug does not reproduce the segfault, so I can't get a full backtrace, all I can get is __fu3634_jl_pgcstack () from /home/Tony/julia/usr/lib/julia/sys.DLL

@tkelman
Member
tkelman commented Oct 22, 2014

I might be nuts, but it seems to have gone away after rebuilding a few times. Carry on, I guess?

@jakebolewski
Member

I've also ran into bugs when trying to profile this branch. Changing the profiling sampling interval fixes it but I don't encounter this behavior on master.

@JeffBezanson JeffBezanson merged commit ecf5426 into master Oct 23, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@JeffBezanson
Member

Jake and I looked at this and the issue seems to be the Expr constructor. The Expr constructor used to be a magic "builtin", but now constructors go through call so there is unavoidably a generic function involved. And generic function methods cannot be "builtins". Something is getting pessimized in there somewhere, very likely involving varargs.

@tkelman
Member

When you add DLLEXPORT, make sure to do so both on the implementation and the prototype - it's an error for them to be inconsistent with MSVC

@tkelman
Member

same here, also needs DLLEXPORT on the prototype in julia.h - I'll go ahead and add it

@Jutho
Contributor
Jutho commented Oct 23, 2014

Hooray, thanks for this. Constructors for abstract types is the first step towards a nicer implementation of 'staged types' ( #8472 ).

@JeffBezanson JeffBezanson deleted the jb/call_constructors branch Oct 25, 2014
@stevengj stevengj added a commit to stevengj/julia that referenced this pull request Oct 31, 2014
@stevengj stevengj fix #8712: invoke for overloaded call 70782b8
@ViralBShah
Member

I don't think this is documented yet, but I might have missed it. If it indeed needs to be done, I guess we should open a new issue.

@stevengj stevengj added a commit to stevengj/julia that referenced this pull request Apr 29, 2016
@stevengj stevengj regression test for #8712, from #8862 da35760
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment