Segfault using StaticArrays and broadcasting #28981
Comments
|
Curiously if you call using LinearAlgebra
using StaticArrays
struct A{N,T}
v::SVector{N,SVector{2,T}}
function A{N,T}(X) where {N,T}
new(normalize.(perp.(X)))
end
end
A(X) = A{length(X),eltype(eltype(X))}(X)
X = rand(SVector{3,SVector{2,Float64}})
A(X) # undefvar error
perp(v) = SVector(v[2], -v[1])
A(X) # now it works |
|
There is something going wrong with the inference of the broadcast: julia> using LinearAlgebra
julia> using StaticArrays
julia> X = rand(SVector{3,SVector{2,Float64}})
3-element SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}:
[0.44029, 0.858698]
[0.0988428, 0.976235]
[0.117725, 0.184207]
julia> foo(X) = normalize.(X)
foo (generic function with 1 method)
julia> @code_warntype foo(X)
Body::SArray{Tuple{3},Any,1,3}
1 1 ─ (Base.ifelse)(false, 0, 3) │╻╷╷╷╷╷╷╷╷ materialize
│ %2 = invoke StaticArrays._broadcast(LinearAlgebra.normalize::typeof(normalize), $(QuoteNode(Size(3,)))::Size{(3,)}, (Size(3,),)::Tuple{Size{(3,)}}, _2::SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3})::SArray{Tuple{3},Any,1,3}
└── return %2 │
julia> foo(X)
3-element SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}:
[0.456261, 0.889846]
[0.100734, 0.994913]
[0.538511, 0.842619]
julia> foo(X) isa SArray{Tuple{3},Any,1,3}
falseThe crash then results from working with the wrongly inferred result. E.g. bar(X) = foo(X)[1]
bar(X) |
|
Note that |
|
Trying to break down and minimize this further, I've noticed that the return value often changes to be of the inferred type, i.e. (outer) eltype Lines 1167 to 1170 in 49e58ba |
|
Oh, yeah, you should never use |
|
I have a hard time condensing this down to a self-contained example, and StaticArrays is probably not the best test vehicle here, but I don't see why the same issue shouldn't show up when broadcasting over an empty array where asking Under the assumption that inference may produce a tighter result once some nested recursive calls have been inferred individually, but should not widen its result, I've tried this patch: --- a/base/compiler/tfuncs.jl
+++ b/base/compiler/tfuncs.jl
@@ -1193,7 +1193,7 @@ function return_type_tfunc(argtypes::Vector{Any}, vtypes::VarTable, sv::Inferenc
return Const(typeof(rt.val))
elseif issingletontype(rt) || rt === Bottom
# output type was known for certain
- return Const(rt)
+ return (rt === Bottom || isconcretetype(rt)) ? Const(rt) : Type{<:rt}
elseif (isa(tt, Const) || isconstType(tt)) &&
(isa(aft, Const) || isconstType(aft))
# input arguments were known for certainIt does fix this issue and does not seem to break anything critical. Maybe the condition could even be relaxed to |
|
I'm quite out of my depth in this discussion, but in addition to type inference correctness (/not segfaulting) there's also potential performance hits if type inference terminates before nailing down concrete types for everything. Does the patch above (inferring eltype as I was hoping that this patch in diff --git a/src/broadcast.jl b/src/broadcast.jl
index 39bf91f..824e06c 100644
--- a/src/broadcast.jl
+++ b/src/broadcast.jl
@@ -126,7 +126,14 @@ scalar_getindex(x::Tuple{<: Any}) = x[1]
end
eltype_exprs = [t <: Union{AbstractArray, Ref} ? :(eltype($t)) : :($t) for t ∈ a]
- newtype_expr = :(return_type(f, Tuple{$(eltype_exprs...)}))
+ isconcretetype_exprs = [:(isconcretetype($t)) for t ∈ eltype_exprs]
+ newtype_expr = isempty(exprs) ? :(return_type(f, Tuple{$(eltype_exprs...)})) : quote
+ if all(($(isconcretetype_exprs...),))
+ typeof($(first(exprs)))
+ else
+ return_type(f, Tuple{$(eltype_exprs...)})
+ end
+ end
return quote
@_inline_metaresulting in julia> @code_warntype A{3,Float64}(X)
Body::A{3,Float64}
5 1 ─ %1 = (Core.tuple)(X)::Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}} │╻ broadcasted
│ %2 = %new(Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}}, perp, %1, nothing)::Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}}
│ %3 = (Core.tuple)(%2)::Tuple{Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}}}
│ (Base.ifelse)(false, 0, 3) ││╻╷╷╷╷╷╷╷╷╷╷╷ instantiate
│ %5 = %new(Base.OneTo{Int64}, 3)::Base.OneTo{Int64} │││┃│││││││││ combine_axes
│ %6 = (Core.tuple)(%5)::Tuple{Base.OneTo{Int64}} ││││┃││││││ broadcast_axes
│ %7 = %new(Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Tuple{Base.OneTo{Int64}},typeof(normalize),Tuple{Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}}}}, LinearAlgebra.normalize, %3, %6)::Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Tuple{Base.OneTo{Int64}},typeof(normalize),Tuple{Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}}}}
│ %8 = %new(getfield(Base.Broadcast, Symbol("##7#8")){Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}},getfield(Base.Broadcast, Symbol("##9#10")){getfield(Base.Broadcast, Symbol("##11#12"))},getfield(Base.Broadcast, Symbol("##13#14")){getfield(Base.Broadcast, Symbol("##15#16"))},getfield(Base.Broadcast, Symbol("##5#6")){getfield(Base.Broadcast, Symbol("##3#4"))}}, %2, getfield(Base.Broadcast, Symbol("##9#10")){getfield(Base.Broadcast, Symbol("##11#12"))}(getfield(Base.Broadcast, Symbol("##11#12"))()), getfield(Base.Broadcast, Symbol("##13#14")){getfield(Base.Broadcast, Symbol("##15#16"))}(getfield(Base.Broadcast, Symbol("##15#16"))()), getfield(Base.Broadcast, Symbol("##5#6")){getfield(Base.Broadcast, Symbol("##3#4"))}(getfield(Base.Broadcast, Symbol("##3#4"))()))::getfield(Base.Broadcast, Symbol("##7#8")){Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}},getfield(Base.Broadcast, Symbol("##9#10")){getfield(Base.Broadcast, Symbol("##11#12"))},getfield(Base.Broadcast, Symbol("##13#14")){getfield(Base.Broadcast, Symbol("##15#16"))},getfield(Base.Broadcast, Symbol("##5#6")){getfield(Base.Broadcast, Symbol("##3#4"))}}
│ %9 = %new(getfield(Base.Broadcast, Symbol("##1#2")){Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Tuple{Base.OneTo{Int64}},typeof(normalize),Tuple{Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}}}},getfield(Base.Broadcast, Symbol("##7#8")){Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}},getfield(Base.Broadcast, Symbol("##9#10")){getfield(Base.Broadcast, Symbol("##11#12"))},getfield(Base.Broadcast, Symbol("##13#14")){getfield(Base.Broadcast, Symbol("##15#16"))},getfield(Base.Broadcast, Symbol("##5#6")){getfield(Base.Broadcast, Symbol("##3#4"))}}}, %7, %8)::getfield(Base.Broadcast, Symbol("##1#2")){Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Tuple{Base.OneTo{Int64}},typeof(normalize),Tuple{Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}}}},getfield(Base.Broadcast, Symbol("##7#8")){Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}},getfield(Base.Broadcast, Symbol("##9#10")){getfield(Base.Broadcast, Symbol("##11#12"))},getfield(Base.Broadcast, Symbol("##13#14")){getfield(Base.Broadcast, Symbol("##15#16"))},getfield(Base.Broadcast, Symbol("##5#6")){getfield(Base.Broadcast, Symbol("##3#4"))}}}
│ %10 = invoke StaticArrays._broadcast(%9::getfield(Base.Broadcast, Symbol("##1#2")){Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Tuple{Base.OneTo{Int64}},typeof(normalize),Tuple{Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}}}},getfield(Base.Broadcast, Symbol("##7#8")){Base.Broadcast.Broadcasted{StaticArrays.StaticArrayStyle{1},Nothing,typeof(perp),Tuple{SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}}},getfield(Base.Broadcast, Symbol("##9#10")){getfield(Base.Broadcast, Symbol("##11#12"))},getfield(Base.Broadcast, Symbol("##13#14")){getfield(Base.Broadcast, Symbol("##15#16"))},getfield(Base.Broadcast, Symbol("##5#6")){getfield(Base.Broadcast, Symbol("##3#4"))}}}, $(QuoteNode(Size(3,)))::Size{(3,)}, (Size(3,),)::Tuple{Size{(3,)}}, _2::SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3})::SArray{Tuple{3},_1,1,3} where _1
│ %11 = (isa)(%10, SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3})::Bool │
└── goto #3 if not %11 │
2 ─ %13 = π (%10, SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}) │
└── goto #4 │
3 ─ %15 = (Base.convert)(SArray{Tuple{3},SArray{Tuple{2},Float64,1,2},1,3}, %10)::SArray{Tuple{3},_1,1,3} where _1
└── goto #4 │
4 ┄ %17 = φ (#2 => %13, #3 => %15)::SArray{Tuple{3},_1,1,3} where _1 │
│ %18 = %new(A{3,Float64}, %17)::A{3,Float64} │
└── return %18 with |
|
Your patch also evaluates the first element twice. I've open a PR with an alternative at JuliaArrays/StaticArrays.jl#495. It leads to the same incomplete inference as yours, however, which presently seems unavoidable for nested broadcasts. EDIT: Note that base julia> X = [rand(3) for i in 1:3]
3-element Array{Array{Float64,1},1}:
[0.127849, 0.334689, 0.304509]
[0.556885, 0.216156, 0.899888]
[0.650899, 0.783833, 0.960853]
julia> bar(x) = 1.0 .* x
bar (generic function with 1 method)
julia> foo(X) = bar.(X)
foo (generic function with 1 method)
julia> @code_warntype foo(X)
Body::Any
1 1 ─ %1 = (Core.tuple)(X)::Tuple{Array{Array{Float64,1},1}} │╻ broadcasted
│ %2 = (Base.arraysize)(X, 1)::Int64 ││╻╷╷╷╷ instantiate
│ %3 = (Base.slt_int)(%2, 0)::Bool │││╻╷╷╷ combine_axes
│ %4 = (Base.ifelse)(%3, 0, %2)::Int64 ││││┃│││││ broadcast_axes
│ %5 = %new(Base.OneTo{Int64}, %4)::Base.OneTo{Int64} │││││┃│││ axes
│ %6 = (Core.tuple)(%5)::Tuple{Base.OneTo{Int64}} ││││││┃ map
│ %7 = %new(Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1},Tuple{Base.OneTo{Int64}},typeof(bar),Tuple{Array{Array{Float64,1},1}}}, bar, %1, %6)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1},Tuple{Base.OneTo{Int64}},typeof(bar),Tuple{Array{Array{Float64,1},1}}}
│ %8 = invoke Base.Broadcast.copy(%7::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1},Tuple{Base.OneTo{Int64}},typeof(bar),Tuple{Array{Array{Float64,1},1}}})::Any
└── return %8 │
julia> foo(X)
3-element Array{Array{Float64,1},1}:
[0.127849, 0.334689, 0.304509]
[0.556885, 0.216156, 0.899888]
[0.650899, 0.783833, 0.960853](Here, inference is even worse, it's not even |
|
I've reduced this to an example which does not depend on StaticArrays: struct SVec1{T} <: AbstractVector{T}
data::T
end
Base.getindex(v::SVec1, i::Int) = v.data
Base.size(a::SVec1) where L = tuple(1)
using Base.Broadcast: BroadcastStyle, AbstractArrayStyle, Broadcasted
struct StaticVectorStyle <: AbstractArrayStyle{1} end
StaticVectorStyle(::Val{1}) = StaticVectorStyle()
BroadcastStyle(::Type{<:SVec1}) = StaticVectorStyle()
# copy overload
function Base.copy(B::Broadcasted{StaticVectorStyle})
flat = Broadcast.flatten(B); as = flat.args; f = flat.f
_broadcast(f, as...)
end
@generated function _broadcast(f, a1::Float64, a2::SVec1)
return quote
T = Core.Compiler.return_type(f, Tuple{typeof(a1), eltype(typeof(a2))})
@inbounds return SVec1{T}(f(a1, a2[1]))
end
end
X = SVec1{SVec1{Float64}}(SVec1{Float64}(0.0))
foo(X) = 1.0 .* X
@show foo(X)
@show typeof(foo(X))
@show Base.return_types(foo, Tuple{typeof(X)})
@show foo(X) isa Base.return_types(foo, Tuple{typeof(X)})[1]Running this gives foo(X) = SVec1{Float64}[[0.0]]
typeof(foo(X)) = SVec1{SVec1{Float64}}
Base.return_types(foo, Tuple{typeof(X)}) = Any[SVec1{Any}]
foo(X) isa (Base.return_types(foo, Tuple{typeof(X)}))[1] = falseIf Let me see whether I can get the broadcast machinery out of the picture... |
|
Eventually we'll probably try to fix this and instead guarantee that this doesn't get inferred. Removing the label, since currently, calls to |
|
Would a solution in the spirit of #28981 (comment) be a viable middle ground? I.e. only infer Meanwhile, this would be a quite self-contained reproducer: struct SVec1{T}
data::T
end
doit(f, args) = _doit(f, args...)
@generated function _doit(f, a::SVec1{E}) where E
return quote
T = Core.Compiler.return_type(f, Tuple{E})
return SVec1{T}(f(a.data))
end
end
bar(X::SVec1) = doit(bar, (X,))
bar(X::Float64) = X
X = SVec1{SVec1{Float64}}(SVec1{Float64}(0.0))
@show bar(X)
@show typeof(bar(X))
@show Base.return_types(bar, Tuple{typeof(X)})
@show bar(X) isa Base.return_types(bar, Tuple{typeof(X)})[1](With the same effect of removing the |
|
It's up to the user to not use |
|
I believe this has been fixed for a while. |
From JuliaArrays/StaticArrays.jl#482, running
we get
The text was updated successfully, but these errors were encountered: