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

Lack-of-inlining bug with type that has extra parameter #18222

Closed
timholy opened this issue Aug 24, 2016 · 6 comments · Fixed by #18225
Closed

Lack-of-inlining bug with type that has extra parameter #18222

timholy opened this issue Aug 24, 2016 · 6 comments · Fixed by #18225
Labels
bug Indicates an unexpected problem or unintended behavior types and dispatch Types, subtyping and method dispatch

Comments

@timholy
Copy link
Member

timholy commented Aug 24, 2016

This is on 0.5.0-rc3:

julia> versioninfo()
Julia Version 0.5.0-rc3+0
Commit e6f843b* (2016-08-22 23:43 UTC)
Platform Info:
  System: Linux (x86_64-linux-gnu)
  CPU: Intel(R) Core(TM) i7-5500U CPU @ 2.40GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Haswell)
  LAPACK: libopenblas64_
  LIBM: libopenlibm
  LLVM: libLLVM-3.7.1 (ORCJIT, broadwell)

Suppose we have some types that we want to scan for in a tuple:

immutable Triggs end
immutable TriggsContainer{T,V}
    obj::V
end

hastriggs(kernel::Tuple) = _hastriggs(false, kernel...)
_hastriggs(ret) = ret
@inline _hastriggs(ret, kern, kernel...) = _hastriggs(ret, kernel...)
@inline _hastriggs{T,V<:Triggs}(ret, kern::Union{V,TriggsContainer{T,V}}, kernel...) = true

If you run julia normally, this works fine:

tim@diva:/tmp$ julia -q
julia> include("inlining.jl")
_hastriggs (generic function with 3 methods)

julia> hastriggs((rand(2), rand(2)))
false

julia> hastriggs((rand(2), Triggs()))
true

julia> hastriggs((Triggs(), rand(2)))
true

But woe be to the person who dares turn off inlining:

tim@diva:/tmp$ julia -q --inline=no
julia> include("inlining.jl")
_hastriggs (generic function with 3 methods)

julia> hastriggs((rand(2), rand(2)))
false

julia> hastriggs((rand(2), Triggs()))
false

julia> hastriggs((Triggs(), rand(2)))
false

If you get rid of the (here useless, but not in my real application)T parameter, then it works correctly with or without inlining.

Interestingly, with inlining off we have:

julia> @which _hastriggs(false, Triggs())
ERROR: no unique matching method for the specified argument types
 in error(::String) at ./error.jl:21
 in which(::Any, ::Any) at ./reflection.jl:442
 in eval(::Module, ::Any) at ./boot.jl:234

and it seems that it likes either one:

julia> methods(_hastriggs, Tuple{Bool, Triggs})
#2 methods for generic function "_hastriggs":
_hastriggs{T,V<:Triggs}(ret, kern::Union{TriggsContainer{T,V},V}, kernel...) at /tmp/inlining.jl:9
_hastriggs(ret, kern, kernel...) at /tmp/inlining.jl:8
@timholy timholy added bug Indicates an unexpected problem or unintended behavior types and dispatch Types, subtyping and method dispatch labels Aug 24, 2016
@timholy
Copy link
Member Author

timholy commented Aug 24, 2016

Interestingly, the @which component of this persists even when inlining is on.

@vtjnash
Copy link
Member

vtjnash commented Aug 24, 2016

Looks like an inliner bug, since the correct answer is false in all of the examples given.

@timholy
Copy link
Member Author

timholy commented Aug 24, 2016

Am I blind? (Probably, yes.) Note that Union includes V<:Triggs as one of the options.

@vtjnash
Copy link
Member

vtjnash commented Aug 24, 2016

yes, but it also has T as one of the requirements, and that can't be satisfied by Triggs

@timholy
Copy link
Member Author

timholy commented Aug 24, 2016

So this is really the same as #18110.

@vtjnash
Copy link
Member

vtjnash commented Aug 24, 2016

similar, yes. I have a patch for this bug and am just writing the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants