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.

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

Jump to bottom

inference: allocate CodeInstance with inference results ASAP #54709

Merged
merged 3 commits into from
Jun 14, 2024
Merged

inference: allocate CodeInstance with inference results ASAP #54709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a68261
inference: allocate CodeInstance with inference results ASAP
vtjnash Jun 6, 2024
77d216b
fix several caching bugs introduced by CodeInstance/CodeInfo rewrite
vtjnash Jun 7, 2024
5cc5640
drop unused field from CodeInstance
vtjnash Jun 7, 2024
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
8 changes: 4 additions & 4 deletions base/boot.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,12 +503,12 @@ end
function CodeInstance(
mi::MethodInstance, owner, @nospecialize(rettype), @nospecialize(exctype), @nospecialize(inferred_const),
@nospecialize(inferred), const_flags::Int32, min_world::UInt, max_world::UInt,
ipo_effects::UInt32, effects::UInt32, @nospecialize(analysis_results),
relocatability::UInt8, edges::DebugInfo)
effects::UInt32, @nospecialize(analysis_results),
relocatability::UInt8, edges::Union{DebugInfo,Nothing})
return ccall(:jl_new_codeinst, Ref{CodeInstance},
(Any, Any, Any, Any, Any, Any, Int32, UInt, UInt, UInt32, UInt32, Any, UInt8, Any),
(Any, Any, Any, Any, Any, Any, Int32, UInt, UInt, UInt32, Any, UInt8, Any),
mi, owner, rettype, exctype, inferred_const, inferred, const_flags, min_world, max_world,
ipo_effects, effects, analysis_results, relocatability, edges)
effects, analysis_results, relocatability, edges)
end
GlobalRef(m::Module, s::Symbol) = ccall(:jl_module_globalref, Ref{GlobalRef}, (Any, Any), m, s)
Module(name::Symbol=:anonymous, std_imports::Bool=true, default_names::Bool=true) = ccall(:jl_f_new_module, Ref{Module}, (Any, Bool, Bool), name, std_imports, default_names)
Expand Down
1 change: 1 addition & 0 deletions base/compiler/compiler.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const setproperty! = Core.setfield!
const swapproperty! = Core.swapfield!
const modifyproperty! = Core.modifyfield!
const replaceproperty! = Core.replacefield!
const _DOCS_ALIASING_WARNING = ""
vtjnash marked this conversation as resolved.
Show resolved Hide resolved

ccall(:jl_set_istopmod, Cvoid, (Any, Bool), Compiler, false)

Expand Down
133 changes: 75 additions & 58 deletions base/compiler/typeinfer.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,8 @@ function typeinf(interp::AbstractInterpreter, frame::InferenceState)
end
end

function finish!(interp::AbstractInterpreter, caller::InferenceState)
function finish!(interp::AbstractInterpreter, caller::InferenceState;
can_discard_trees::Bool=may_discard_trees(interp))
result = caller.result
valid_worlds = result.valid_worlds
if last(valid_worlds) >= get_world_counter()
Expand All @@ -228,6 +229,43 @@ function finish!(interp::AbstractInterpreter, caller::InferenceState)
if opt isa OptimizationState
result.src = opt = ir_to_codeinf!(opt)
end
if isdefined(result, :ci)
inferred_result = nothing
relocatability = 0x1
const_flag = is_result_constabi_eligible(result)
if is_cached(caller) || !can_discard_trees
ci = result.ci
if !(is_result_constabi_eligible(result) && can_discard_trees)
inferred_result = transform_result_for_cache(interp, result.linfo, result.valid_worlds, result, can_discard_trees)
relocatability = 0x0
if inferred_result isa CodeInfo
edges = inferred_result.debuginfo
uncompressed = inferred_result
inferred_result = maybe_compress_codeinfo(interp, result.linfo, inferred_result, can_discard_trees)
result.is_src_volatile |= uncompressed !== inferred_result
elseif ci.owner === nothing
# The global cache can only handle objects that codegen understands
inferred_result = nothing
end
if isa(inferred_result, String)
t = @_gc_preserve_begin inferred_result
relocatability = unsafe_load(unsafe_convert(Ptr{UInt8}, inferred_result), Core.sizeof(inferred_result))
@_gc_preserve_end t
elseif inferred_result === nothing
relocatability = 0x1
end
end
# n.b. relocatability = (isa(inferred_result, String) && inferred_result[end]) || inferred_result === nothing
end
if !@isdefined edges
edges = DebugInfo(result.linfo)
end
ccall(:jl_update_codeinst, Cvoid, (Any, Any, Int32, UInt, UInt, UInt32, Any, UInt8, Any),
ci, inferred_result, const_flag,
first(result.valid_worlds), last(result.valid_worlds),
encode_effects(result.ipo_effects), result.analysis_results,
relocatability, edges)
end
return nothing
end

Expand All @@ -236,7 +274,6 @@ function _typeinf(interp::AbstractInterpreter, frame::InferenceState)
# with no active ip's, frame is done
frames = frame.callers_in_cycle
if isempty(frames)
push!(frames, frame)
finish_nocycle(interp, frame)
elseif length(frames) == 1
@assert frames[1] === frame "invalid callers_in_cycle"
Expand All @@ -250,15 +287,13 @@ end

function finish_nocycle(::AbstractInterpreter, frame::InferenceState)
frame.dont_work_on_me = true
finish(frame, frame.interp)
finishinfer!(frame, frame.interp)
opt = frame.result.src
if opt isa OptimizationState # implies `may_optimize(caller.interp) === true`
optimize(frame.interp, opt, frame.result)
end
finish!(frame.interp, frame)
if is_cached(frame)
cache_result!(frame.interp, frame.result)
end
unlock_mi_inference(frame.interp, frame.linfo)
vtjnash marked this conversation as resolved.
Show resolved Hide resolved
return nothing
end

Expand All @@ -277,7 +312,7 @@ function finish_cycle(::AbstractInterpreter, frames::Vector{InferenceState})
end
for caller in frames
adjust_cycle_frame!(caller, cycle_valid_worlds, cycle_valid_effects)
finish(caller, caller.interp)
finishinfer!(caller, caller.interp)
end
for caller in frames
opt = caller.result.src
Expand All @@ -287,9 +322,7 @@ function finish_cycle(::AbstractInterpreter, frames::Vector{InferenceState})
end
for caller in frames
finish!(caller.interp, caller)
if is_cached(caller)
cache_result!(caller.interp, caller.result)
end
unlock_mi_inference(caller.interp, caller.linfo)
end
return nothing
end
Expand All @@ -313,8 +346,7 @@ function is_result_constabi_eligible(result::InferenceResult)
result_type = result.result
return isa(result_type, Const) && is_foldable_nothrow(result.ipo_effects) && is_inlineable_constant(result_type.val)
end
function CodeInstance(interp::AbstractInterpreter, result::InferenceResult;
can_discard_trees::Bool=may_discard_trees(interp))
function CodeInstance(interp::AbstractInterpreter, result::InferenceResult)
local const_flags::Int32
result_type = result.result
@assert !(result_type === nothing || result_type isa LimitedAccuracy)
Expand Down Expand Up @@ -348,38 +380,11 @@ function CodeInstance(interp::AbstractInterpreter, result::InferenceResult;
end
relocatability = 0x0
owner = cache_owner(interp)
if const_flags == 0x3 && can_discard_trees
inferred_result = nothing
relocatability = 0x1
else
inferred_result = transform_result_for_cache(interp, result.linfo, result.valid_worlds, result, can_discard_trees)
if inferred_result isa CodeInfo
edges = inferred_result.debuginfo
uncompressed = inferred_result
inferred_result = maybe_compress_codeinfo(interp, result.linfo, inferred_result, can_discard_trees)
result.is_src_volatile |= uncompressed !== inferred_result
elseif owner === nothing
# The global cache can only handle objects that codegen understands
inferred_result = nothing
end
if isa(inferred_result, String)
t = @_gc_preserve_begin inferred_result
relocatability = unsafe_load(unsafe_convert(Ptr{UInt8}, inferred_result), Core.sizeof(inferred_result))
@_gc_preserve_end t
elseif inferred_result === nothing
relocatability = 0x1
end
end
# n.b. relocatability = (isa(inferred_result, String) && inferred_result[end]) || inferred_result === nothing
if !@isdefined edges
edges = DebugInfo(result.linfo)
end
return CodeInstance(result.linfo, owner,
widenconst(result_type), widenconst(result.exc_result), rettype_const, inferred_result,
widenconst(result_type), widenconst(result.exc_result), rettype_const, nothing,
const_flags, first(result.valid_worlds), last(result.valid_worlds),
# TODO: Actually do something with non-IPO effects
encode_effects(result.ipo_effects), encode_effects(result.ipo_effects), result.analysis_results,
relocatability, edges)
encode_effects(result.ipo_effects), result.analysis_results,
relocatability, nothing)
end

function transform_result_for_cache(interp::AbstractInterpreter,
Expand Down Expand Up @@ -417,22 +422,22 @@ function cache_result!(interp::AbstractInterpreter, result::InferenceResult)
result.valid_worlds = WorldRange(first(result.valid_worlds), typemax(UInt))
end
# check if the existing linfo metadata is also sufficient to describe the current inference result
# to decide if it is worth caching this
# to decide if it is worth caching this right now
mi = result.linfo
already_inferred = already_inferred_quick_test(interp, mi)
cache_results = !already_inferred_quick_test(interp, mi)
cache = WorldView(code_cache(interp), result.valid_worlds)
if !already_inferred && haskey(cache, mi)
if cache_results && haskey(cache, mi)
ci = cache[mi]
# Even if we already have a CI for this, it's possible that the new CI has more
# information (E.g. because the source was limited before, but is no longer - this
# happens during bootstrap). In that case, allow the result to be recached.
if result.src === nothing || (ci.inferred !== nothing || ci.invoke != C_NULL)
already_inferred = true
# n.b.: accurate edge representation might cause the CodeInstance for this to be constructed later
if isdefined(ci, :inferred)
cache_results = false
end
end

# TODO: also don't store inferred code if we've previously decided to interpret this function
if !already_inferred
if cache_results
code_cache(interp)[mi] = ci = CodeInstance(interp, result)
result.ci = ci
if track_newly_inferred[]
Expand All @@ -442,15 +447,18 @@ function cache_result!(interp::AbstractInterpreter, result::InferenceResult)
end
end
end
unlock_mi_inference(interp, mi)
nothing
end

function cycle_fix_limited(@nospecialize(typ), sv::InferenceState)
if typ isa LimitedAccuracy
if sv.parent === nothing
# when part of a cycle, we might have unintentionally introduced a limit marker
@assert !isempty(sv.callers_in_cycle)
# we might have introduced a limit marker, but we should know it must be sv and other callers_in_cycle
#@assert !isempty(sv.callers_in_cycle)
# FIXME: this assert fails, appearing to indicate there is a bug in filtering this list earlier.
# In particular (during doctests for example), during inference of
# show(Base.IOContext{Base.GenericIOBuffer{Memory{UInt8}}}, Base.Multimedia.MIME{:var"text/plain"}, LinearAlgebra.BunchKaufman{Float64, Array{Float64, 2}, Array{Int64, 1}})
# we observed one of the ssavaluetypes here to be Core.Compiler.LimitedAccuracy(typ=Any, causes=Core.Compiler.IdSet(getproperty(LinearAlgebra.BunchKaufman{Float64, Array{Float64, 2}, Array{Int64, 1}}, Symbol)))
Comment on lines +456 to +461
Copy link
Member

Choose a reason for hiding this comment

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

AFAIU, this assertion should be failing because of the removal of push!(frames, frame) on L239, i.e. it fails when LimitedAccuracy is introduced in cases of self-recursion. Was this an intentional change?

Copy link
Member Author

@vtjnash vtjnash Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The code at L239 seemed to have been making this accidentally pass and hiding this bug. In particular, there didn't seem to be self-recursion here (and LimitedAccuracy should not occur if there is self-recursion, typically)?

return typ.typ
end
causes = copy(typ.causes)
Expand Down Expand Up @@ -561,7 +569,7 @@ end

# inference completed on `me`
# update the MethodInstance
function finish(me::InferenceState, interp::AbstractInterpreter)
function finishinfer!(me::InferenceState, interp::AbstractInterpreter)
# prepare to run optimization passes on fulltree
s_edges = me.stmt_edges[1]
if s_edges === nothing
Expand Down Expand Up @@ -606,7 +614,6 @@ function finish(me::InferenceState, interp::AbstractInterpreter)
me.result.src = nothing
me.cache_mode = CACHE_MODE_NULL
set_inlineable!(me.src, false)
unlock_mi_inference(interp, me.linfo)
elseif limited_src
# a type result will be cached still, but not this intermediate work:
# we can throw everything else away now
Expand All @@ -630,6 +637,10 @@ function finish(me::InferenceState, interp::AbstractInterpreter)
end

maybe_validate_code(me.linfo, me.src, "inferred")

if is_cached(me)
cache_result!(me.interp, me.result)
end
nothing
end

Expand Down Expand Up @@ -993,7 +1004,7 @@ function codeinstance_for_const_with_code(interp::AbstractInterpreter, code::Cod
src = codeinfo_for_const(interp, code.def, code.rettype_const)
return CodeInstance(code.def, cache_owner(interp), code.rettype, code.exctype, code.rettype_const, src,
Int32(0x3), code.min_world, code.max_world,
code.ipo_purity_bits, code.purity_bits, code.analysis_results,
code.ipo_purity_bits, code.analysis_results,
code.relocatability, src.debuginfo)
end

Expand Down Expand Up @@ -1155,7 +1166,7 @@ function typeinf_ext(interp::AbstractInterpreter, mi::MethodInstance, source_mod
src isa CodeInfo || return nothing
return CodeInstance(mi, cache_owner(interp), Any, Any, nothing, src, Int32(0),
get_inference_world(interp), get_inference_world(interp),
UInt32(0), UInt32(0), nothing, UInt8(0), src.debuginfo)
UInt32(0), nothing, UInt8(0), src.debuginfo)
end
end
lock_mi_inference(interp, mi)
Expand All @@ -1178,9 +1189,15 @@ function typeinf_ext(interp::AbstractInterpreter, mi::MethodInstance, source_mod
# Inference result is not cacheable or is was cacheable, but we do not want to
# store the source in the cache, but the caller wanted it anyway (e.g. for reflection).
# We construct a new CodeInstance for it that is not part of the cache hierarchy.
can_discard_trees = source_mode ≠ SOURCE_MODE_FORCE_SOURCE &&
can_discard_trees = source_mode == SOURCE_MODE_NOT_REQUIRED ||
is_result_constabi_eligible(result)
code = CodeInstance(interp, result; can_discard_trees)
code = CodeInstance(interp, result)
if source_mode == SOURCE_MODE_ABI
# XXX: this should be using the CI from the cache, if possible: haskey(cache, mi) && (ci = cache[mi])
code_cache(interp)[mi] = code
end
result.ci = code
finish!(interp, frame; can_discard_trees)

# If the caller cares about the code and this is constabi, still use our synthesis function
# anyway, because we will have not finished inferring the code inside the CodeInstance once
Expand Down
2 changes: 1 addition & 1 deletion src/codegen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9875,7 +9875,7 @@ jl_llvm_functions_t jl_emit_codeinst(
else if (jl_is_method(def) && // don't delete toplevel code
def->source != NULL && // don't delete code from optimized opaque closures that can't be reconstructed
inferred != jl_nothing && // and there is something to delete (test this before calling jl_ir_inlining_cost)
!effects_foldable(codeinst->ipo_purity_bits) && // don't delete code we may want for irinterp
!effects_foldable(jl_atomic_load_relaxed(&codeinst->ipo_purity_bits)) && // don't delete code we may want for irinterp
((jl_ir_inlining_cost(inferred) == UINT16_MAX) || // don't delete inlineable code
jl_atomic_load_relaxed(&codeinst->invoke) == jl_fptr_const_return_addr) && // unless it is constant
!(params.imaging_mode || jl_options.incremental)) { // don't delete code when generating a precompile file
Expand Down
Loading