-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix new type instability from getindex(::String, r::UnitRange{Int})
#55625
Conversation
We can add this regression test: let m = only(methods(Base._string_n, (Integer,)))
mi = Core.Compiler.specialize_method(m, Tuple{typeof(Base._string_n),Integer}, Core.svec())
m′ = only(methods(Base.getindex, (String,UnitRange{Int})))
mi′ = Core.Compiler.specialize_method(m′, Tuple{typeof(Base.getindex),String,UnitRange{Int}}, Core.svec())
@test mi′ ∉ mi.backedges
end but I have doubts about whether it will remain a robust test case in the future. It's probably safer to use tools like JET for testing this sort of things. |
In my PR this reproducer is provided: using SnoopCompileCore
length(@snoop_invalidations begin
struct I <: Integer end
Csize_t
Csize_t(::I) = Csize_t(0)
end) Is there a way to count invalidations that could be used from within the test suite? Could we add a test something like: @test length(invalidations) < 50 ? |
The number of invalidations isn't that robust and SnoopCompile isn't available for Julia base test suite at this moment. |
After investigating the cause of the invalidation reported in #55583, it was found that the issue arises only when `r` is propagated as an extended lattice element, such as `PartialStruct(UnitRange{Int}, ...)`, for the method of `getindex(::String, r::UnitRange{Int})`. Specifically, the path at https://github.com/JuliaLang/julia/blob/cebfd7bc66153b82c56715cb1cb52dac7df8eac8/base/compiler/typeinfer.jl#L809-L815 is hit, so the direct cause was the recursion limit for constant inference. To explain in more detail, within the slow path of `nextind` which is called inside `getindex(::String, ::UnitRange{Int})`, the 1-argument `@assert` is used https://github.com/JuliaLang/julia/blob/cebfd7bc66153b82c56715cb1cb52dac7df8eac8/base/strings/string.jl#L211. The code related to `print` associated with this `@assert` further uses `getindex(::String, ::UnitRange{Int})`, causing the recursion limit. This recursion limit is only for constant inference, which is why we saw this regression only for the `PartialStruct` case. Moreover, since this recursion limit occurs within the `@assert`-related code, this issue did not arise until now (i.e. until #49260 was merged). As a solution, I considered improving the recursion limit itself, but decided that keeping the current code for the recursion limit of constant inference is safer. Ideally, this should be addressed on the compiler side, but there is certainly deep recursion in this case. As an easier solution, this commit resolves the issue by changing the 1-arg `@assert` to the 2-arg version. - replaces #55583
c750629
to
d87f645
Compare
After investigating the cause of the invalidation reported in #55583, it was found that the issue arises only when
r
is propagated as an extended lattice element, such asPartialStruct(UnitRange{Int}, ...)
, for the method ofgetindex(::String, r::UnitRange{Int})
. Specifically, the path atjulia/base/compiler/typeinfer.jl
Lines 809 to 815 in cebfd7b
To explain in more detail, within the slow path of
nextind
which is called insidegetindex(::String, ::UnitRange{Int})
, the 1-argument@assert
is usedjulia/base/strings/string.jl
Line 211 in cebfd7b
print
associated with this@assert
further usesgetindex(::String, ::UnitRange{Int})
, causing the recursion limit. This recursion limit is only for constant inference, which is why we saw this regression only for thePartialStruct
case. Moreover, since this recursion limit occurs within the@assert
-related code, this issue did not arise until now (i.e. until #49260 was merged).As a solution, I considered improving the recursion limit itself, but decided that keeping the current code for the recursion limit of constant inference is safer. Ideally, this should be addressed on the compiler side, but there is certainly deep recursion in this case. As an easier solution, this commit resolves the issue by changing the 1-arg
@assert
to the 2-arg version.Integer
subtype #55583