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

Handle non-const function in return_types special case #19667

Merged
merged 1 commit into from
Dec 29, 2016

Conversation

yuyichao
Copy link
Contributor

Fix #19641

@yuyichao yuyichao added the compiler:inference Type inference label Dec 21, 2016
@yuyichao
Copy link
Contributor Author

@nanosoldier runbenchmarks(ALL, vs = ":master")

@yuyichao yuyichao added the needs tests Unit tests are required for this change label Dec 21, 2016
@Sacha0
Copy link
Member

Sacha0 commented Dec 21, 2016

8127269

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@Sacha0
Copy link
Member

Sacha0 commented Dec 21, 2016

I could not reproduce any of the possible regressions locally. Best!

@Sacha0
Copy link
Member

Sacha0 commented Dec 21, 2016

Confirming the test case in #19633 (comment) succeeds on this branch:

julia> baz() = (a = 1; Base._return_type((x, y) -> x + y + a, Base.Broadcast.typestuple(eye(4), eye(4))))
baz (generic function with 1 method)

julia> baz()
Float64

julia> @code_warntype baz()
Variables:
  #self#::#baz
  a::Int64
  #3::##3#4{Int64}

Body:
  begin
      a::Int64 = 1
      SSAValue(0) = Base._return_type
      #3::##3#4{Int64} = $(Expr(:new, ##3#4{Int64}, :(a)))
      SSAValue(1) = #3::##3#4{Int64}
      $(Expr(:invoke, MethodInstance for eye(::Type{T}, ::Int64, ::Int64), :(Base.eye), :(Base.Float64), 4, 4))
      $(Expr(:invoke, MethodInstance for eye(::Type{T}, ::Int64, ::Int64), :(Base.eye), :(Base.Float64), 4, 4))
      SSAValue(2) = $(QuoteNode(Tuple{Float64,Float64}))
      return $(QuoteNode(Float64))
  end::Type{Float64}

Best!

@musm
Copy link
Contributor

musm commented Dec 21, 2016

ldexp, frexp, exponent, and significand are notoriously noisy. The functions are evaluated on scalars and take a few ns to finish. BenchmarkTools locally has ns resolution on my local pc, but perhaps not on the benchmarking server

@yuyichao yuyichao force-pushed the yyc/typeinf/return_type branch from 93bc498 to b94d546 Compare December 21, 2016 19:00
@yuyichao yuyichao removed the needs tests Unit tests are required for this change label Dec 21, 2016
@jrevels
Copy link
Member

jrevels commented Dec 21, 2016

BenchmarkTools locally has ns resolution on my local pc, but perhaps not on the benchmarking server

The latest BenchmarkTools tag has picosecond resolution (it will pretty-print in fractions of nanoseconds), but I still need to update Nanosoldier with that capability.

@Sacha0
Copy link
Member

Sacha0 commented Dec 21, 2016

Confirming that the test cases I've written against the code mentioned in the related discourse thread succeed on this branch. Best!

@andyferris
Copy link
Member

Is this backportable?

(@tkelman are there any backports planned for v0.5? we still haven't seen v0.5.1, but then again I haven't seen a lot of bugs either)

@yuyichao yuyichao force-pushed the yyc/typeinf/return_type branch from b94d546 to 5a063ec Compare December 23, 2016 14:42
@yuyichao
Copy link
Contributor Author

Rebased. AppVeyor failure is the intermittent libgit2 network issue..... @JeffBezanson @vtjnash

Sacha0 added a commit to Sacha0/julia that referenced this pull request Dec 25, 2016
…re cases.

Re-simplify broadcast's eltype promotion mechanism as in JuliaLang#19421. With benefit of JuliaLang#19667, this simplified mechanism should handle additional cases (e.g. closures accepting more than two arguments). Also rename the mechanism more precisely (_broadcast_type -> _broadcast_eltype).
@yuyichao
Copy link
Contributor Author

Ping @JeffBezanson @vtjnash

@JeffBezanson JeffBezanson merged commit ba9fe91 into master Dec 29, 2016
@yuyichao yuyichao deleted the yyc/typeinf/return_type branch December 29, 2016 17:43
Sacha0 added a commit to Sacha0/julia that referenced this pull request Dec 30, 2016
…re cases.

Re-simplify broadcast's eltype promotion mechanism as in JuliaLang#19421. With benefit of JuliaLang#19667, this simplified mechanism should handle additional cases (e.g. closures accepting more than two arguments). Also rename the mechanism more precisely (_broadcast_type -> _broadcast_eltype).
Sacha0 added a commit to Sacha0/julia that referenced this pull request Dec 30, 2016
…re cases.

Re-simplify broadcast's eltype promotion mechanism as in JuliaLang#19421. With benefit of JuliaLang#19667, this simplified mechanism should handle additional cases (e.g. closures accepting more than two arguments). Also rename the mechanism more precisely (_broadcast_type -> _broadcast_eltype).
Sacha0 added a commit to Sacha0/julia that referenced this pull request Dec 31, 2016
…re cases.

Re-simplify broadcast's eltype promotion mechanism as in JuliaLang#19421. With benefit of JuliaLang#19667, this simplified mechanism should handle additional cases (e.g. closures accepting more than two arguments). Also rename the mechanism more precisely (_broadcast_type -> _broadcast_eltype).
Sacha0 added a commit to Sacha0/julia that referenced this pull request Dec 31, 2016
…re cases.

Re-simplify broadcast's eltype promotion mechanism as in JuliaLang#19421. With benefit of JuliaLang#19667, this simplified mechanism should handle additional cases (e.g. closures accepting more than two arguments). Also rename the mechanism more precisely (_broadcast_type -> _broadcast_eltype).
Sacha0 added a commit to Sacha0/julia that referenced this pull request Dec 31, 2016
…re cases.

Re-simplify broadcast's eltype promotion mechanism as in JuliaLang#19421. With benefit of JuliaLang#19667, this simplified mechanism should handle additional cases (e.g. closures accepting more than two arguments). Also rename the mechanism more precisely (_broadcast_type -> _broadcast_eltype).
Sacha0 added a commit to Sacha0/julia that referenced this pull request Jan 1, 2017
…re cases.

Re-simplify broadcast's eltype promotion mechanism as in JuliaLang#19421. With benefit of JuliaLang#19667, this simplified mechanism should handle additional cases (e.g. closures accepting more than two arguments). Also rename the mechanism more precisely (_broadcast_type -> _broadcast_eltype, typestuple -> eltypestuple).
Sacha0 added a commit to Sacha0/julia that referenced this pull request Jan 1, 2017
…re cases.

Re-simplify broadcast's eltype promotion mechanism as in JuliaLang#19421. With benefit of JuliaLang#19667, this simplified mechanism should handle additional cases (e.g. closures accepting more than two arguments). Also rename the mechanism more precisely (_broadcast_type -> _broadcast_eltype, typestuple -> eltypestuple).
tkelman pushed a commit that referenced this pull request Jan 1, 2017
…re cases. (#19723)

Re-simplify broadcast's eltype promotion mechanism as in #19421. With benefit of #19667, this simplified mechanism should handle additional cases (e.g. closures accepting more than two arguments). Also rename the mechanism more precisely (_broadcast_type -> _broadcast_eltype, typestuple -> eltypestuple).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants