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

Type instability with broadcast #1078

Closed
sharanry opened this issue Aug 16, 2022 · 3 comments · Fixed by #1079
Closed

Type instability with broadcast #1078

sharanry opened this issue Aug 16, 2022 · 3 comments · Fixed by #1079

Comments

@sharanry
Copy link

Stable:

julia> f1(t) = map(t->t / (1 - t ^ 2), t)
f1 (generic function with 1 method)

julia> @inferred f1(SVector(rand(3)...))
3-element SVector{3, Float64} with indices SOneTo(3):
 0.43576925170786196
 0.9981057701031397
 0.2526628536334409

Type-unstable:

julia> f2(t) = t ./ (1 .- t .^ 2)
f2 (generic function with 1 method)

julia> @inferred f2(SVector(rand(3)...))
ERROR: return type SVector{3, Float64} does not match inferred return type SVector{3}
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] top-level scope
   @ REPL[24]:1

Related: SciML/Integrals.jl#94

CC: @YingboMa @ChrisRackauckas

@mateuszbaran
Copy link
Collaborator

At least part of the issue with complicated broadcasts (that example is complicated enough) and StaticArrays is that Julia gives up way too easily when faced with method instance recursion inlining. My attempts at reducing the amount of method instance recursion in broadcast code weren't met with much enthusiasm, and I haven't seen any improvements on the compiler side (very recent master branch of Julia has the same issue).

@N5N3
Copy link
Contributor

N5N3 commented Aug 17, 2022

JuliaLang/julia#43322 should be enough to fix all related problems.
The fix only works on >v1.6 as const propation is much less agressive there.

Do we want to port that design into StaticArrays.jl? (I'm affraid that PR wont be merged as sooner, and even its merged, we still need a solution for previous julia versions.)

@mateuszbaran
Copy link
Collaborator

mateuszbaran commented Aug 17, 2022

So I'm not the only one who is trying to solve that issue 🙂 . Your changes look fine at the first glance so I'd review them for StaticArrays.jl and be OK with merging them, though it would have to be limited to StaticArrayStyle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants