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

Converting StaticInt to a different type causes invalidations #52

Open
DhairyaLGandhi opened this issue Apr 19, 2022 · 7 comments
Open

Comments

@DhairyaLGandhi
Copy link
Member

(::Type{T})(x::StaticInt{N}) where {T<:Integer,N} = T(N)

This definition causes conversion of the types of (::Type{T})(::Integer) to be invalidated because StaticInt <: Integer. Since Flux uses ArrayInterface, it pulls in Static and shows up the invalidations.

List of invalidations as seen in a session with Flux, but most of the methods seem be in Compiler, Base and StdLibs

inserting (::Type{T})(x::Static.StaticInt{N}) where {T<:Integer, N} in Static at /home/dhairyalgandhi/.julia/packages/Static/pkxBE
/src/int.jl:26 invalidated:
   mt_backedges:  1: signature Tuple{Type{UInt64}, Integer} triggered MethodInstance for convert(::Type{UInt64}, ::Integer) (0 chi
ldren)
                  2: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for resize!(::BitVector, ::Integer) (0 childre
n)
                  3: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for findnext(::Pkg.Types.var"#23#24"{String},
::AbstractString, ::Integer) (0 children)
                  4: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for convert(::Type{Int64}, ::Integer) (0 children)
                  5: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for findprev(::InteractiveUtils.var"#34#39", ::AbstractString, ::Integer) (0 children)
                  6: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for insert!(::BitVector, ::Integer, ::Any) (0 children)
                  7: signature Tuple{Type{Int32}, Integer} triggered MethodInstance for Revise.revise_dir_queued(::String) (0 children)
                  8: signature Tuple{Type{Int32}, Integer} triggered MethodInstance for Core.Compiler.add_remark!(::GPUCompiler.GPUInterpreter, ::Core.Compiler.InferenceState, ::String) (0 children)
                  9: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for GPUCompiler.ci_cache_populate(::GPUCompiler.GPUInterpreter, ::GPUCompiler.CodeCache, ::Core.MethodTable, ::Core.MethodInstance, ::Any, ::Int32) (0 children)
                 10: signature Tuple{Type{UInt64}, Integer} triggered MethodInstance for CUDA.decode(::Vector{CUDA.CUjit_option_enum}, ::Vector{Ptr{Nothing}}) (0 children)
                 11: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for CUDA.decode(::Vector{CUDA.CUjit_option_enum}, ::Vector{Ptr{Nothing}}) (0 children)
                 12: signature Tuple{Type{UInt32}, Integer} triggered MethodInstance for VersionNumber(::Integer, ::Int64, ::Int64, ::Tuple{}, ::Tuple{}) (1 children)
                 13: signature Tuple{Type{Int32}, Integer} triggered MethodInstance for Int32(::Enum) (1 children)
                 14: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for nextind(::AbstractVector, ::Integer) (1 children)
                 15: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for Int64(::Enum) (1 children)
                 16: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for TimerOutputs.rpad(::String, ::Integer, ::C
har) (2 children)
                 17: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for TimerOutputs.rpad(::String, ::Integer, ::S
tring) (3 children)
                 18: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for LoweredCodeUtils.find_typedefs(::Core.Code
Info) (23 children)
                 19: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for Base.to_shape(::Integer) (34 children)
@chriselrod
Copy link
Collaborator

The irritating thing here is that if we remove the definition, we get a MethodError on trying to call Int(static(3)), so I don't know what we can do within Static.jl to actually address this.
Seems like the best solution may be to modify each of the invalidated methods in Base Julia instead.

@Tokazama
Copy link
Collaborator

Tokazama commented Apr 19, 2022

Would that just be a PR with a bunch of @nospecialize?
as in resize!(x::BitVector, @nospecialize(n::Integer)).

@Tokazama
Copy link
Collaborator

I opened up a draft PR for this. I welcome any ideas to get this issue resolved.
JuliaLang/julia#45094

@ChrisRackauckas
Copy link
Member

maybe require to_int as a separate function instead?

@Tokazama
Copy link
Collaborator

If we remove these two it fixes it:

Base.convert(::Type{T}, @nospecialize(N::StaticInt)) where {T<:Number} = convert(T, Int(N))
(::Type{T})(@nospecialize(x::StaticInt)) where {T<:Integer} = T(known(x))

but it also leads to a lot of other stuff not working anymore.

We also would benefit from this PR.

@chriselrod
Copy link
Collaborator

chriselrod commented May 21, 2022

@Tokazama Mind making a PR with a breaking version bump that gets rid of all the invalidations?

We can have an ugly API, but I think under the view that static is for internal/library use, we can go with fully explicit APIs.
If someone is deliberately using ArrayInterface.size instead of Base.size, they're doing it in expectation of getting static results, and can write uglier code accordingly.

We should be willing to optimize libraries for performance (load and run time), even if it's at the expense of beauty.
That said, things hopefully won't be uglier than necessary.

But explicit/easy to read isn't necessarily ugly/has its own advantages for maintainability.

@Tokazama
Copy link
Collaborator

Sure I can do that, but as a heads up it's going to be a bit of a mess for a while. It's essentially the same as removing the Integer subtyping because without the ability to generically convert to Int it just fails to work in a lot of cases.

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

4 participants