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.

Sign up for GitHub

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

Jump to bottom

Regression of inferrability of broadcast code #51129

Closed
giordano opened this issue Aug 31, 2023 · 1 comment · Fixed by #51154
Closed

Regression of inferrability of broadcast code #51129

giordano opened this issue Aug 31, 2023 · 1 comment · Fixed by #51154
Labels
broadcast Applying a function over a collection compiler:inference Type inference regression Regression in behavior compared to a previous version
Milestone
1.10

Comments

@giordano
Copy link
Contributor

giordano commented Aug 31, 2023 •
edited
Loading

In v1.10.0-beta2 I get

julia> f(v, x) = (1 .- (v ./ x) .^ 2)
f (generic function with 1 method)

julia> code_warntype(f, Tuple{Vector{Float64}, Float64})
MethodInstance for f(::Vector{Float64}, ::Float64)
  from f(v, x) @ Main REPL[1]:1
Arguments
  #self#::Core.Const(f)
  v::Vector{Float64}
  x::Float64
Body::Union{Vector, BitVector}
1 ─ %1 = Main.:-::Core.Const(-)
│   %2 = Main.:^::Core.Const(^)
│   %3 = Base.broadcasted(Main.:/, v, x)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}
│   %4 = Core.apply_type(Base.Val, 2)::Core.Const(Val{2})
│   %5 = (%4)()::Core.Const(Val{2}())
│   %6 = Base.broadcasted(Base.literal_pow, %2, %3, %5)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}
│   %7 = Base.broadcasted(%1, 1, %6)::Core.PartialStruct(Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(-), Tuple{Int64, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}}}, Any[Core.Const(Base.Broadcast.DefaultArrayStyle{1}()), Core.Const(-), Core.PartialStruct(Tuple{Int64, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}}, Any[Core.Const(1), Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}]), Nothing])
│   %8 = Base.materialize(%7)::Union{Vector, BitVector}
└──      return %8

Return type of f is inferred as Union{Vector, BitVector}, but I'm missing how this can be a BitVector at all.

Interestingly, if I change the definition of f to f(v, x) = ((v ./ x) .^ 2) and then change back to f(v, x) = (1 .- (v ./ x) .^ 2) then this is inferred correctly as Vector{Float64}:

julia> f(v, x) = ((v ./ x) .^ 2)
f (generic function with 1 method)

julia> code_warntype(f, Tuple{Vector{Float64}, Float64})
MethodInstance for f(::Vector{Float64}, ::Float64)
  from f(v, x) @ Main REPL[5]:1
Arguments
  #self#::Core.Const(f)
  v::Vector{Float64}
  x::Float64
Body::Vector{Float64}
1 ─ %1 = Main.:^::Core.Const(^)
│   %2 = Base.broadcasted(Main.:/, v, x)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}
│   %3 = Core.apply_type(Base.Val, 2)::Core.Const(Val{2})
│   %4 = (%3)()::Core.Const(Val{2}())
│   %5 = Base.broadcasted(Base.literal_pow, %1, %2, %4)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}
│   %6 = Base.materialize(%5)::Vector{Float64}
└──      return %6


julia> f(v, x) = (1 .- (v ./ x) .^ 2)
f (generic function with 1 method)

julia> code_warntype(f, Tuple{Vector{Float64}, Float64})
MethodInstance for f(::Vector{Float64}, ::Float64)
  from f(v, x) @ Main REPL[7]:1
Arguments
  #self#::Core.Const(f)
  v::Vector{Float64}
  x::Float64
Body::Vector{Float64}
1 ─ %1 = Main.:-::Core.Const(-)
│   %2 = Main.:^::Core.Const(^)
│   %3 = Base.broadcasted(Main.:/, v, x)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}
│   %4 = Core.apply_type(Base.Val, 2)::Core.Const(Val{2})
│   %5 = (%4)()::Core.Const(Val{2}())
│   %6 = Base.broadcasted(Base.literal_pow, %2, %3, %5)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}
│   %7 = Base.broadcasted(%1, 1, %6)::Core.PartialStruct(Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(-), Tuple{Int64, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}}}, Any[Core.Const(Base.Broadcast.DefaultArrayStyle{1}()), Core.Const(-), Core.PartialStruct(Tuple{Int64, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}}, Any[Core.Const(1), Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}]), Nothing])
│   %8 = Base.materialize(%7)::Vector{Float64}
└──      return %8

The fact that this depends on previous definitions of the function is quite surprising.

In 1.9.3 I get out-of-the-box the return type Vector{Float64}

julia> code_warntype(f, Tuple{Vector{Float64}, Float64})
MethodInstance for f(::Vector{Float64}, ::Float64)
  from f(v, x) @ Main REPL[1]:1
Arguments
  #self#::Core.Const(f)
  v::Vector{Float64}
  x::Float64
Body::Vector{Float64}
1 ─ %1 = Main.:-::Core.Const(-)
│   %2 = Main.:^::Core.Const(^)
│   %3 = Base.broadcasted(Main.:/, v, x)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}
│   %4 = Core.apply_type(Base.Val, 2)::Core.Const(Val{2})
│   %5 = (%4)()::Core.Const(Val{2}())
│   %6 = Base.broadcasted(Base.literal_pow, %2, %3, %5)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}
│   %7 = Base.broadcasted(%1, 1, %6)::Core.PartialStruct(Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(-), Tuple{Int64, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}}}, Any[Core.Const(-), Core.PartialStruct(Tuple{Int64, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}}, Any[Core.Const(1), Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(Base.literal_pow), Tuple{Base.RefValue{typeof(^)}, Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(/), Tuple{Vector{Float64}, Float64}}, Base.RefValue{Val{2}}}}]), Core.Const(nothing)])
│   %8 = Base.materialize(%7)::Vector{Float64}
└──      return %8

as expected.
@giordano giordano added regression Regression in behavior compared to a previous version broadcast Applying a function over a collection compiler:inference Type inference labels Aug 31, 2023
@giordano giordano added this to the 1.10 milestone Aug 31, 2023
@giordano giordano mentioned this issue Aug 31, 2023
Merged
@vtjnash
Copy link
Member

vtjnash commented Aug 31, 2023

Apparently inference cannot deal with the ntuple version of this, since it sees confusing recursion in the abstractinterpretation, so it doesn't attempt to constprop the values for i into the getindex function call here. So we should perhaps go back to the recursive version for now:

diff --git a/base/broadcast.jl b/base/broadcast.jl
index fd330a7f2c..43044f9b7d 100644
--- a/base/broadcast.jl
+++ b/base/broadcast.jl
@@ -714,8 +714,8 @@ _broadcast_getindex_eltype(A) = eltype(A)  # Tuple, Array, etc.
 eltypes(::Tuple{}) = Tuple{}
 eltypes(t::Tuple{Any}) = Iterators.TupleOrBottom(_broadcast_getindex_eltype(t[1]))
 eltypes(t::Tuple{Any,Any}) = Iterators.TupleOrBottom(_broadcast_getindex_eltype(t[1]), _broadcast_getindex_eltype(t[2]))
-# eltypes(t::Tuple) = (TT = eltypes(tail(t)); TT === Union{} ? Union{} : Iterators.TupleOrBottom(_broadcast_getindex_eltype(t[1]), TT.parameters...))
-eltypes(t::Tuple) = Iterators.TupleOrBottom(ntuple(i -> _broadcast_getindex_eltype(t[i]), Val(length(t)))...)
+eltypes(t::Tuple) = (TT = eltypes(tail(t)); TT === Union{} ? Union{} : Iterators.TupleOrBottom(_broadcast_getindex_eltype(t[1]), TT.parameters...))
+# eltypes(t::Tuple) = Iterators.TupleOrBottom(ntuple(i -> _broadcast_getindex_eltype(t[i]), Val(length(t)))...)
 
 # Inferred eltype of result of broadcast(f, args...)
 function combine_eltypes(f, args::Tuple)

@vtjnash vtjnash mentioned this issue Aug 31, 2023
Closed
vtjnash added a commit that referenced this issue Aug 31, 2023
@vtjnash
broadcast: use recursion rather than ntuple to map over a tuple
5d91f14 
Inference seems to have trouble with the anonymous function version, so
go back to the recursive version.

Fixes #51129
Probably also fixes #50859
@vtjnash vtjnash mentioned this issue Sep 1, 2023
Merged
vtjnash added a commit that referenced this issue Sep 1, 2023
@vtjnash
broadcast: use recursion rather than ntuple to map over a tuple
8ea7ffc 
Inference seems to have trouble with the anonymous function version, so
go back to the recursive version.

Fixes #51129
Probably also fixes #50859
N5N3 pushed a commit that referenced this issue Sep 3, 2023
@vtjnash
broadcast: use recursion rather than ntuple to map over a tuple (#51154)
d949bb4 
Inference seems to have trouble with the anonymous function version, so
go back to the recursive version.

Fixes #51129
Probably also fixes #50859
KristofferC pushed a commit that referenced this issue Sep 15, 2023
@vtjnash
broadcast: use recursion rather than ntuple to map over a tuple (#51154)
8332712 
Inference seems to have trouble with the anonymous function version, so
go back to the recursive version.

Fixes #51129
Probably also fixes #50859

(cherry picked from commit d949bb4)
nalimilan pushed a commit that referenced this issue Nov 5, 2023
@vtjnash @nalimilan
broadcast: use recursion rather than ntuple to map over a tuple (#51154)
e88ceda 
Inference seems to have trouble with the anonymous function version, so
go back to the recursive version.

Fixes #51129
Probably also fixes #50859

(cherry picked from commit d949bb4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
broadcast Applying a function over a collection compiler:inference Type inference regression Regression in behavior compared to a previous version
Projects
None yet
Milestone
1.10
Development

Successfully merging a pull request may close this issue.

broadcast: use recursion rather than ntuple to map over a tuple JuliaLang/julia
2 participants
@vtjnash @giordano