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

Static evaluation of method_exists (when true) #16422

Closed
wants to merge 3 commits into from
Closed

Conversation

timholy
Copy link
Member

@timholy timholy commented May 18, 2016

Given that I encouraged @nalimilan to shoot for #16401, I felt I should chip in by trying to figure out how to evaluate method_exists at compile time. Also of apparent interest to @stevengj (#7088).

I tried to simply follow #7088, but there are clearly some differences. Here's my test script:

function foo(a, b)
    if ccall(:jl_method_exists, Cint, (Any, Any), typeof(-).name.mt, Tuple{typeof(-), typeof(a), typeof(b)}) != 0
        return a - b
    end
    b
end

function foo2(a, b)
    if my_method_exists(-, (typeof(a), typeof(b)))
        return a - b
    end
    b
end

@inline function my_method_exists(f, t)
    return ccall(:jl_method_exists, Cint, (Any, Any), typeof(f).name.mt, makett(f, t)) != 0
end

using Base.@pure

@pure function makett(f, t)
    t1 = Base.to_tuple_type(t)
    Tuple{isa(f,Type) ? Type{f} : typeof(f), t1.parameters...}
end

Following with gdb, it's clear that foo works (confirmed by @code_native, though in contrast with isleaftype this happy outcome is not revealed with @code_warntype). However, with foo2 it doesn't. It also doesn't work if I evaluate makett and store it in a variable before issuing the ccall. Do I have to statically evaluate the call to makett, or am I barking up the wrong tree here?

@yuyichao
Copy link
Contributor

codegen is too late for this to help type inference.

@yuyichao
Copy link
Contributor

Can you just make method_exists @pure? It should be apart from #265 and that's effectively what you are assuming here anyway?

@timholy
Copy link
Member Author

timholy commented May 18, 2016

codegen is too late for this to help type inference.

It's not to help type inference (that's already done), it's to avoid the run-time method lookup.

Can you just make method_exists @pure?

If I put @pure in front of my_method_exists, then @code_native says no.

@yuyichao
Copy link
Contributor

#!/usr/bin/julia -f

using Base.@pure

function foo(a, b)
    if ccall(:jl_method_exists, Cint, (Any, Any), typeof(-).name.mt, Tuple{typeof(-), typeof(a), typeof(b)}) != 0
        return a - b
    end
    b
end

function foo2(a, b)
    if my_method_exists(-, Tuple{typeof(a), typeof(b)})
        return a - b
    end
    b
end

@pure function my_method_exists(f, t)
    return ccall(:jl_method_exists, Cint, (Any, Any),
                 typeof(f).name.mt, makett(f, t)) != 0
end

@pure function makett(f, t)
    t1 = Base.to_tuple_type(t)
    Tuple{isa(f,Type) ? Type{f} : typeof(f), t1.parameters...}
end

@code_warntype foo2(1, 2)
@code_llvm foo2(1, 2)
Variables:
  #self#::#foo2
  a::Int64
  b::Int64

Body:
  begin  # /home/yuyichao/tmp/julia/a.jl, line 13:
      $(QuoteNode(true)) # /home/yuyichao/tmp/julia/a.jl, line 14:
      return (Base.box)(Int64,(Base.sub_int)(a::Int64,b::Int64))
  end::Int64

define i64 @julia_foo2_49878(i64, i64) #0 !dbg !6 {
top:
  %2 = sub i64 %0, %1
  ret i64 %2
}

Tuple of types can't be inferred as good as tuple types (const prop should in principle work though.)

@yuyichao
Copy link
Contributor

It's not to help type inference (that's already done), it's to avoid the run-time method lookup.

My help type inference I mean help const prop (and dead code elimination) in type inference.

@timholy
Copy link
Member Author

timholy commented May 18, 2016

That's sweet! I should have tested Tuple{} vs ().

Presumably one only wants to "inline" the answer when it evaluates as true. I'll try combining your solution and mine.

@timholy timholy changed the title WIP: static evaluation of method_exists (when true) Static evaluation of method_exists (when true) May 19, 2016
@timholy
Copy link
Member Author

timholy commented May 19, 2016

OK, this proved to be a little subtle. The fact that you only want to evaluate this at compile time when it evaluates to true means that you can't use @pure. The fact that a ccall wants expressions for all of its arguments makes this difficult/impossible to encapsulate in a function. So, I introduced a macro @method_exists to be used when you want this to evaluate at compile time, when true.

Now including docs and tests. I believe this is ready to merge.

@yuyichao
Copy link
Contributor

The fact that you only want to evaluate this at compile time when it evaluates to true

Why is that?

@timholy
Copy link
Member Author

timholy commented May 19, 2016

Because I'm hoping to pay the taxidermy fees for whoever wants to stuff and mount #265 on their wall. I didn't want to introduce another mechanism by which behavior depends on the order in which functions are defined and run.

@yuyichao
Copy link
Contributor

I don't think this will make #265 much worse and this is also the behavior we want in the long term...

@timholy
Copy link
Member Author

timholy commented May 19, 2016

Are you saying we'll also start compiling in throw(MethodError(...)) if the function doesn't exist yet?

@yuyichao
Copy link
Contributor

Yes and only recompile the method when you hit the global scope again.

true
```
"""
:@method_exists
Copy link
Contributor

Choose a reason for hiding this comment

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

would be better to move these inline rather than adding to this file

@timholy
Copy link
Member Author

timholy commented May 19, 2016

Wow, that's a huge change. This is happening soon enough that I/you can go ahead and just do the @pure implemenation? (Feel free to do it yourself, since you suggested it, or I will if you'd prefer.)

@vtjnash
Copy link
Member

vtjnash commented May 19, 2016

I don't think this is @pure, which makes me hesitate to add this right now.

@mschauer
Copy link
Contributor

Currently this can only have a one-sided solution. There might be a place for language to state that a function is "intentionally left blank", something like size(x::Number) = nomethod() which then would produce a method error on call, a warning if a method is (re-)defined and would return method_exists(...)==false

@StefanKarpinski
Copy link
Member

If we had that it would probably be better spelled as @nomethod size(x::Number) but it feels weirdly redundant. No kidding there's no method – we didn't define one!

@timholy
Copy link
Member Author

timholy commented May 24, 2016

During my travels I missed @vtjnash's point. Sounds like I should rebase this and merge? (EDIT: in case it's not clear, this solution doesn't use @pure; it's @yuyichao's that does.)

@mschauer
Copy link
Contributor

Not really redundant per se, rather an alternative to defining traits like HasThatfunction() giving the same information. I read it as promise (to be broken at one's own risk) not to monkeypatch a certain method which then allows full static evaluation of method_exists.

@timholy
Copy link
Member Author

timholy commented Jun 1, 2016

Rebased. Since this implementation doesn't use @pure, I'm going to merge this soon in the absence of other objections.

}
}
else if (e1 && (jl_is_datatype(e1) || jl_is_typename(e1))) {
return jl_get_field(e1, jl_symbol_name(s));
Copy link
Member

Choose a reason for hiding this comment

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

not all of the fields in typename are constant

Copy link
Member Author

@timholy timholy Jun 1, 2016

Choose a reason for hiding this comment

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

This only needs the mt field, is that safe? (It also only needs the name field of a datatype, so if there's any danger there I could restrict it.) EDIT: of course the method table changes as you add new methods, but presumably the table itself is constant?

Copy link
Member

Choose a reason for hiding this comment

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

No, the table itself is not constant

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah. To me that sounds like "we should close this without merging." Or, if the overall functionality is desirable, "find a way to do it that doesn't involve jl_static_eval, so it doesn't imply this is safe generally."

@vtjnash
Copy link
Member

vtjnash commented Mar 3, 2023

Now implemented (though possibly documented wrongly now)

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