Skip to content

staticdata: Insert backedges recursively #57212

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 31, 2025
Merged

staticdata: Insert backedges recursively #57212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 17 additions & 22 deletions base/staticdata.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,26 +39,16 @@ function _insert_backedges(edges::Vector{Any}, stack::Vector{CodeInstance}, visi
verify_method_graph(codeinst, stack, visiting)
minvalid = codeinst.min_world
maxvalid = codeinst.max_world
if maxvalid ≥ minvalid
if get_world_counter() == maxvalid
# if this callee is still valid, add all the backedges
Base.Compiler.store_backedges(codeinst, codeinst.edges)
end
if get_world_counter() == maxvalid
maxvalid = typemax(UInt)
@atomic :monotonic codeinst.max_world = maxvalid
end
if external
caller = get_ci_mi(codeinst)
@assert isdefined(codeinst, :inferred) # See #53586, #53109
inferred = @ccall jl_rettype_inferred(
codeinst.owner::Any, caller::Any, minvalid::UInt, maxvalid::UInt)::Any
if inferred !== nothing
# We already got a code instance for this world age range from
# somewhere else - we don't need this one.
else
@ccall jl_mi_cache_insert(caller::Any, codeinst::Any)::Cvoid
end
if maxvalid ≥ minvalid && external
caller = get_ci_mi(codeinst)
@assert isdefined(codeinst, :inferred) # See #53586, #53109
inferred = @ccall jl_rettype_inferred(
codeinst.owner::Any, caller::Any, minvalid::UInt, maxvalid::UInt)::Any
if inferred !== nothing
# We already got a code instance for this world age range from
# somewhere else - we don't need this one.
else
@ccall jl_mi_cache_insert(caller::Any, codeinst::Any)::Cvoid
end
end
end
Expand Down Expand Up @@ -196,9 +186,14 @@ function verify_method(codeinst::CodeInstance, stack::Vector{CodeInstance}, visi
while length(stack) ≥ depth
child = pop!(stack)
if maxworld ≠ 0
@atomic :monotonic child.min_world = minworld
@atomic :monotonic child.min_world = minworld
end
if maxworld == current_world
Base.Compiler.store_backedges(child, child.edges)
@atomic :monotonic child.max_world = typemax(UInt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current_world may change while these backedge insertion is occurring. The part of this change that combined these separate atomics checks introduces an unsoundness here. This line needs to be put back into _insert_backedges: first all backedges must be inserted (for the cycle), and then, if the current_world is still the same, it can atomically change max_world => typemax to acknowledge that all of the backedges existed in the current world.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, and you're fine with capping the world age if that happened rather than revalidating?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, yeah, we are supposed to revalidate later if we need this code later. We finally have the code for that written now (this file) but haven't finished hooking that up yet.

else
@atomic :monotonic child.max_world = maxworld
end
@atomic :monotonic child.max_world = maxworld
@assert visiting[child] == length(stack) + 1
delete!(visiting, child)
invalidations = _jl_debug_method_invalidation[]
Expand Down
33 changes: 33 additions & 0 deletions test/precompile.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2222,4 +2222,37 @@ precompile_test_harness("No package module") do load_path
String(take!(io)))
end

precompile_test_harness("Constprop CodeInstance invalidation") do load_path
write(joinpath(load_path, "DefineTheMethod.jl"),
"""
module DefineTheMethod
export the_method
the_method_val(::Val{x}) where {x} = x
the_method_val(::Val{1}) = 0xdeadbeef
the_method_val(::Val{2}) = 2
the_method_val(::Val{3}) = 3
the_method_val(::Val{4}) = 4
the_method_val(::Val{5}) = 5
Base.@constprop :aggressive the_method(x) = the_method_val(Val{x}())
the_method(2)
end
""")
Base.compilecache(Base.PkgId("DefineTheMethod"))
write(joinpath(load_path, "CallTheMethod.jl"),
"""
module CallTheMethod
using DefineTheMethod
call_the_method() = the_method(1)
call_the_method()
end
""")
Base.compilecache(Base.PkgId("CallTheMethod"))
@eval using DefineTheMethod
@eval using CallTheMethod
@eval DefineTheMethod.the_method_val(::Val{1}) = Int(0)
invokelatest() do
@test Int(0) == CallTheMethod.call_the_method()
end
end

finish_precompile_test!()