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

SVector type unstable for Julia v1.9 and --check-bounds=no #1155

Open
sloede opened this issue Apr 24, 2023 · 3 comments
Open

SVector type unstable for Julia v1.9 and --check-bounds=no #1155

sloede opened this issue Apr 24, 2023 · 3 comments

Comments

@sloede
Copy link

sloede commented Apr 24, 2023

As described in JuliaLang/julia#49472, using StaticArrays.jl with Julia v1.9 and --check-bounds=no causes the SVector constructor to be type unstable even for the simplest invocations.

To reproduce, start Julia v1.9 (RC1 or later were tested and reproduce this problem) with --check-bounds=no and then run the following:

julia> using StaticArrays

julia> @code_warntype SVector(1)
MethodInstance for (SVector)(::Int64)
  from (::Type{SA})(x...) where SA<:StaticArray @ StaticArrays ~/hackathon/perf/mwe-staticarrays/rc1/tmp-depot/packages/StaticArrays/4uslg/src/convert.jl:160
Static Parameters
  SA = SVector
Arguments
  #self#::Type{SVector}
  x::Tuple{Int64}
Body::Any
1nothing%2 = $(Expr(:static_parameter, 1))::Core.Const(SVector)
│   %3 = StaticArrays.Args(x)::StaticArrays.Args{Tuple{Int64}}%4 = StaticArrays.construct_type(%2, %3)::Any%5 = (%4)(x)::Any
└──      return %5

Without --check-bounds=no, we get the expected, type stable output:

julia> using StaticArrays

julia> @code_warntype SVector(1)
MethodInstance for (SVector)(::Int64)
  from (::Type{SA})(x...) where SA<:StaticArray @ StaticArrays ~/hackathon/perf/mwe-staticarrays/rc1/tmp-depot/packages/StaticArrays/4uslg/src/convert.jl:160
Static Parameters
  SA = SVector
Arguments
  #self#::Type{SVector}
  x::Tuple{Int64}
Body::SVector{1, Int64}
1nothing%2 = $(Expr(:static_parameter, 1))::Core.Const(SVector)
│   %3 = StaticArrays.Args(x)::StaticArrays.Args{Tuple{Int64}}%4 = StaticArrays.construct_type(%2, %3)::Core.Const(SVector{1, Int64})
│   %5 = (%4)(x)::SVector{1, Int64}
└──      return %5

This regression renders StaticArrays.jl virtually unusable with Julia v1.9 for many HPC workloads (hot kernels that make generous use of SVector basically grind to a halt). In the linked julia issue above it was hinted that this is not going to be fixed in Julia base, and @vchuravy suggested that I should rather file an issue here (keeping fingers crossed 🙏).

cc @ranocha

@vchuravy
Copy link
Contributor

vchuravy commented Apr 24, 2023

using Cthulhu with remarks, and effects on.

Check-bounds=0

∘ ─ %0 = invoke StaticArray(::Int64)::Any (!c,!e,!n,!t,!s,!m,+i)′
1 ─      nothing::Core.Const(nothing)
│   @ /home/vchuravy/.julia/packages/StaticArrays/4uslg/src/convert.jl:160 within `StaticArray`
│   %2 = $(Expr(:static_parameter, 1))::Core.Const(SVector) (+c,+e,+n,+t,+s,+m,+i)
│   %3 = StaticArrays.Args(x)::StaticArrays.Args{Tuple{Int64}} (+c,+e,+n,+t,+s,+m,+i)
│   %4 = StaticArrays.construct_type(%2, %3)::Any (!c,!e,!n,!t,!s,!m,+i)′ Call inference reached maximally imprecise information. Bailing on. [constprop] Disabled by method instance heuristic
│   %5 = (%4)(x)::Any (!c,!e,!n,!t,!s,!m,+i)′ Could not identify method table for call
└──      return %5

Check-bounds=auto

∘ ─ %0 = invoke StaticArray(::Int64)::SVector{1, Int64} (+c,+e,+n,+t,+s,+m,+i)
1 ─      nothing::Core.Const(nothing)
│   @ /home/vchuravy/.julia/packages/StaticArrays/4uslg/src/convert.jl:160 within `StaticArray`
│   %2 = $(Expr(:static_parameter, 1))::Core.Const(SVector) (+c,+e,+n,+t,+s,+m,+i)
│   %3 = StaticArrays.Args(x)::StaticArrays.Args{Tuple{Int64}} (+c,+e,+n,+t,+s,+m,+i)
│   %4 = StaticArrays.construct_type(%2, %3)::Core.Const(SVector{1, Int64}) (+c,+e,+n,+t,+s,+m,+i) [constprop] No more information to be gained
│   %5 = (%4)(x)::SVector{1, Int64} (+c,+e,+n,+t,+s,+m,+i) [constprop] Disabled by argument and rettype heuristics
└──      return %5

@vchuravy
Copy link
Contributor

The failure comes from:

@code_warntype StaticArrays.adapt_size(SVector,StaticArrays.Args((1,)))

Check-bounds=0

9 ┄─ %63 = Base.typeintersect::Core.Const(typeintersect) (+c,+e,+n,+t,+s,+m,+i)
│    %64 = $(Expr(:static_parameter, 1))::Core.Const(SVector) (+c,+e,+n,+t,+s,+m,+i)
│    %65 = StaticArrays.StaticArrayNoEltype::Core.Const(StaticArray{S, T, N} where {S, N, T}) (+c,+e,+n,+t,+s,+m,+i)
│    %66 = SZ::Core.Const(Tuple{1})
│    %67 = StaticArrays.tuple_length(SZ::Core.Const(Tuple{1}))::Core.Const(1) (+c,+e,+n,+t,+s,+m,+i)
│    %68 = Core.apply_type(%65, %66, %67)::Core.Const(StaticArray{Tuple{1}, T, 1} where T) (+c,+e,+n,+t,+s,+m,+i)
│          (SA′ = (%63)(%64, %68))::Any (+c,+e,+n,+t,+s,+m,+i) Call inference reached maximally imprecise information. Bailing on.
│    %70 = SA′::Any

Check-bounds=auto

9 ┄─ %63 = Base.typeintersect::Core.Const(typeintersect) (+c,+e,+n,+t,+s,+m,+i)
│    %64 = $(Expr(:static_parameter, 1))::Core.Const(SVector) (+c,+e,+n,+t,+s,+m,+i)
│    %65 = StaticArrays.StaticArrayNoEltype::Core.Const(StaticArray{S, T, N} where {S, N, T}) (+c,+e,+n,+t,+s,+m,+i)
│    %66 = SZ::Core.Const(Tuple{1})
│    %67 = StaticArrays.tuple_length(SZ::Core.Const(Tuple{1}))::Core.Const(1) (+c,+e,+n,+t,+s,+m,+i)
│    %68 = Core.apply_type(%65, %66, %67)::Core.Const(StaticArray{Tuple{1}, T, 1} where T) (+c,+e,+n,+t,+s,+m,+i)
│          (SA′ = (%63)(%64, %68))::Core.Const(SVector{1}) (+c,+e,+n,+t,+s,+m,+i)
│    %70 = SA′::Core.Const(SVector{1})

@vchuravy
Copy link
Contributor

cc: @aviatesk

Noteworthy difference is:

--check-bounds=auto

   %69 = < concrete eval > typeintersect(::Core.Const(SVector),::Core.Const(StaticArray{Tuple{1}, T, 1} where T))::… (+c,+e,+n,+t,+s,+m,+i)

--check-bounds=false

   %69 = < constprop > typeintersect(::Core.Const(SVector),::Core.Const(StaticArray{Tuple{1}, T, 1} where T))::Any (+c,+e,+n,+t,+s,+m,+i)

aviatesk added a commit to JuliaLang/julia that referenced this issue Jun 8, 2023
From version 1.9 onwards, when `--check-bounds=no` is used,
concrete-eval is completely disabled. However, it appears
`--check-bounds=no` is still being used within the community, causing
issues like the one reported in JuliaArrays/StaticArrays.jl#1155.
Although we should move forward to a direction of eliminating the flag
in the future (#48245), for the time being, there are many requests to
carry out a certain level of compiler optimization, even when this flag
is enabled.

This commit aims to allow concrete-eval "safely" even under
`--check-bounds=no`. Specifically, when the method call being analyzed
is `:nothrow`, it should be predominantly safe to concrete-eval it under
this flag. Technically, however, even `:nothrow` methods could trigger
undefined behavior, since `:nothrow` isn't a strict constraint and it's
possible for users to annotate potentially risky methods with
`Base.@assume_effects :nothrow`. Nonetheless, since this possibility is
acknowledged in `Base.@assume_effects` documentation, I feel it's fair
to relegate it to user responsibility.
vchuravy pushed a commit to JuliaLang/julia that referenced this issue Jun 12, 2023
#50107)

From version 1.9 onwards, when `--check-bounds=no` is used,
concrete-eval is completely disabled. However, it appears
`--check-bounds=no` is still being used within the community, causing
issues like the one reported in JuliaArrays/StaticArrays.jl#1155.
Although we should move forward to a direction of eliminating the flag
in the future (#48245), for the time being, there are many requests to
carry out a certain level of compiler optimization, even when this flag
is enabled.

This commit aims to allow concrete-eval "safely" even under
`--check-bounds=no`. Specifically, when the method call being analyzed
is `:nothrow`, it should be predominantly safe to concrete-eval it under
this flag. Technically, however, even `:nothrow` methods could trigger
undefined behavior, since `:nothrow` isn't a strict constraint and it's
possible for users to annotate potentially risky methods with
`Base.@assume_effects :nothrow`. Nonetheless, since this possibility is
acknowledged in `Base.@assume_effects` documentation, I feel it's fair
to relegate it to user responsibility.
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

No branches or pull requests

2 participants