Inferable broadcast and promote_op of closures #19633

pabloferz · 2016-12-17T15:41:40Z

Restoring some of the behavior before #19421 as it seems to be needed by some packages, e.g. JuliaArrays/StaticArrays.jl#88

Also, should help with the problem in this discourse thread

Sacha0 · 2016-12-17T17:39:08Z

base/broadcast.jl

+typestuple(T::Type) = (Base.@_pure_meta; Tuple{Type{T}})
+typestuple(a, b...) = (Base.@_pure_meta; Tuple{typestuple(a).types..., typestuple(b...).types...})
+
+ftype(f, A) = typeof(a->f(a))


Could typeof(a -> f(a)) simplify to typeof(f)?

'Core.Typeof' should be the equivalent. 'typeof' won't handle the case where T <: Type

@vtjnash, could you expand briefly? IIUC, you mean that ftype(f, A) = Core.Typeof(f) could replace ftype(f, A) = typeof(a -> f(a)) whereas ftype(f, A) = typeof(f) could not, as Core.Typeof(f) will return Type{T} when f::T<:Type whereas typeof(f) will not. Is that correct? If so, would defining ftype(f, A) = typeof(f); ftype(f::Type, A) = Type{T} as in the early version of #19421 (see #19633 (comment)) or instead ftype(f, A) = Core.Typeof(f) be better in this case? Thanks!

We need to handle the cases ftype(f, A...) and ftype(T::Type, A...) separately anyway. Otherwise, Core.Typeof would be have been a would option for ftype(f, A) = typeof(f) and ftype(T::Type, A) = Type{T}

Sacha0 · 2016-12-17T17:48:29Z

base/broadcast.jl

+
+ftype(f, A) = typeof(a->f(a))
+ftype(f, A...) = typeof(a->f(a...))
+ftype(T::Type, A...) = Type{T}


If memory serves, in an early version of #19421 (intermediate commits that no longer appear on github unfortunately) you also defined ftype(T::Type, A) = Type{T} --- was that additional definition unnecessary?

Right, it wasn't needed

Sacha0 · 2016-12-17T17:51:00Z

base/broadcast.jl

+ftype(f, A) = typeof(a->f(a))
+ftype(f, A...) = typeof(a->f(a...))
+ftype(T::Type, A...) = Type{T}
+ziptype(A) = (Base.@_pure_meta; Tuple{eltype(A)})


Did ziptype require another definition as well in the early versions of #19421? I cannot recall unfortunately.

Sacha0 · 2016-12-17T17:55:31Z

base/broadcast.jl

+ziptype(A, B) = (Base.@_pure_meta; Iterators.Zip2{ziptype(A), ziptype(B)})
+@inline ziptype(A, B, C, D...) = Iterators.Zip{ziptype(A),ziptype(B, C, D...)}
+
+_broadcast_type(f, T::Type, As...) = Base._return_type(f, typestuple(T, As...))


I imagine this construction handles the test case for #19421, but will not handle types in other argument positions?

Sacha0 · 2016-12-17T18:01:35Z

base/promotion.jl

 end

-promote_op(::Any...) = (@_pure_meta; Any)


Removing this fallback seems like a step forward (for safety) though somewhat orthogonal to the other changes in this pull request. I imagine this removal impacts other aspects of this pull request in ways I do not immediately see? Thanks!

This was unintentional. Of course, it would be nice to get rid of this, but that would have to be done elsewhere. I'll put this back for now.

Sacha0 · 2016-12-17T18:39:14Z

test/broadcast.jl

+
+let f(A, n) = broadcast(x -> +(x, n), A)
+    @test @inferred(f([1.0], 1)) == [2.0]
+end


Perhaps point to the discourse thread, and include an analog of the direct test of the promotion mechanism from that thread? For example,

# Test issue described in https://discourse.julialang.org/t/towards-broadcast-over-combinations-of-sparse-matrices-and-scalars/910 let f(A, n) = broadcast(x -> +(x, n), A) @test @inferred(f([1.0], 1)) == [2.0] foo() = (a = 1.0; Base.Broadcast._broadcast_type(x -> x + a, 1.0)) @test @inferred(foo()) == Float64 end

Sacha0 · 2016-12-17T18:41:27Z

@pabloferz, wonderful work as usual! Thanks!

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

nanosoldier · 2016-12-17T21:58:56Z

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

Sacha0 · 2016-12-20T05:25:57Z

@pabloferz, another puzzle for you: While one-argument closures now works, closures with more than one argument still fail, e.g. the following two-argument extension of the test case from the relevant discourse thread:

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

julia> foo()
Any

julia> @code_warntype foo()
Variables:
  #self#::#foo
  a::Int64
  #1::##1#2{Int64}

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

(The preceding hits the _default_eltype-first-Generator-zip mechanism.) The _return_type-typestuple mechanism yields the correct result type, but as we've learned is not inferable when closures are involved (ref. discussion in the discourse thread linked above and #19641):

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}))
      $(Expr(:inbounds, false))
      # meta: location promotion.jl _return_type 226
      # meta: location inference.jl return_type 4095
      # meta: location inference.jl Type 20
      SSAValue(4) = (Core.getfield)($(QuoteNode(Core.Box(Core.Inference.#call#112))),:contents)::Any
      # meta: location inference.jl inlining_enabled 1552
      SSAValue(3) = (Core.getfield)((Core.Inference.pointerref)((Core.Inference.cglobal)(:jl_options,Core.Inference.JLOptions)::Ptr{Core.Inference.JLOptions},1,1)::Core.Inference.JLOptions,:can_inline)::Int8
      # meta: pop location
      # meta: pop location
      # meta: pop location
      # meta: pop location
      $(Expr(:inbounds, :pop))
      return ((Core.getfield)(Core.Inference,:return_type)::Core.Inference.#return_type)(SSAValue(1),SSAValue(2),(SSAValue(4))(((Core.Inference.box)(Int64,(Core.Inference.sext_int)(Int64,SSAValue(3)))::Int64 === 1)::Bool,15,4,16,4,Core.Inference.InferenceParams)::Any)::Any
  end::Any

Any thoughts, or are we out of luck till (hopefully) Core.Inference.return_type advances? Thanks again!

pabloferz · 2016-12-20T15:08:27Z

@Sacha0 for those I believe we're stuck until Core.Inference.return_type handles closures properly.

Sacha0 reviewed Dec 17, 2016

View reviewed changes

Make broadcasting closures inferable again

f6e65af

pabloferz force-pushed the pz/ziptype-ftype branch from f687172 to f6e65af Compare December 17, 2016 20:46

kshyatt added the compiler:inference Type inference label Dec 18, 2016

Sacha0 approved these changes Dec 18, 2016

View reviewed changes

Sacha0 added the broadcast Applying a function over a collection label Dec 18, 2016

Sacha0 mentioned this pull request Dec 18, 2016

Core.Inference.return_type and closures #19641

Closed

kshyatt merged commit 706031e into JuliaLang:master Dec 19, 2016

pabloferz deleted the pz/ziptype-ftype branch December 19, 2016 23:40

Sacha0 mentioned this pull request Dec 21, 2016

Handle non-const function in return_types special case #19667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inferable broadcast and promote_op of closures #19633

Inferable broadcast and promote_op of closures #19633

pabloferz commented Dec 17, 2016

Sacha0 Dec 17, 2016

vtjnash Dec 17, 2016

Sacha0 Dec 17, 2016

pabloferz Dec 17, 2016

Sacha0 Dec 17, 2016

pabloferz Dec 17, 2016 •

edited

Loading

Sacha0 Dec 17, 2016

Sacha0 Dec 17, 2016

pabloferz Dec 17, 2016

Sacha0 Dec 17, 2016

pabloferz Dec 17, 2016

Sacha0 Dec 17, 2016

pabloferz Dec 17, 2016

Sacha0 commented Dec 17, 2016

nanosoldier commented Dec 17, 2016

Sacha0 commented Dec 20, 2016

pabloferz commented Dec 20, 2016

Inferable broadcast and promote_op of closures #19633

Inferable broadcast and promote_op of closures #19633

Conversation

pabloferz commented Dec 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabloferz Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 commented Dec 17, 2016

nanosoldier commented Dec 17, 2016

Sacha0 commented Dec 20, 2016

pabloferz commented Dec 20, 2016

pabloferz Dec 17, 2016 •

edited

Loading