Skip to content

Commit

Permalink
fix cases where metadata was not being set correctly
Browse files Browse the repository at this point in the history
  • Loading branch information
vtjnash committed Feb 21, 2024
1 parent f2c6334 commit 7057763
Show file tree
Hide file tree
Showing 11 changed files with 101 additions and 74 deletions.
2 changes: 1 addition & 1 deletion base/compiler/abstractinterpretation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1181,7 +1181,7 @@ function const_prop_methodinstance_heuristic(interp::AbstractInterpreter,
if isa(code, CodeInstance)
inferred = @atomic :monotonic code.inferred
# TODO propagate a specific `CallInfo` that conveys information about this call
if inlining_policy(interp, inferred, NoCallInfo(), IR_FLAG_NULL) !== nothing
if src_inlining_policy(interp, inferred, NoCallInfo(), IR_FLAG_NULL)
return true
end
end
Expand Down
2 changes: 1 addition & 1 deletion base/compiler/inferencestate.jl
Original file line number Diff line number Diff line change
Expand Up @@ -800,7 +800,7 @@ function IRInterpretationState(interp::AbstractInterpreter,
@assert code.def === mi
src = @atomic :monotonic code.inferred
if isa(src, String)
src = _uncompressed_ir(mi.def, src)
src = _uncompressed_ir(code, src)
else
isa(src, CodeInfo) || return nothing
end
Expand Down
16 changes: 9 additions & 7 deletions base/compiler/optimize.jl
Original file line number Diff line number Diff line change
Expand Up @@ -107,21 +107,23 @@ is_declared_noinline(@nospecialize src::MaybeCompressed) =
# OptimizationState #
#####################

function inlining_policy(interp::AbstractInterpreter,
# return whether this src should be inlined. If so, retrieve_ir_for_inlining must return an IRCode from it
function src_inlining_policy(interp::AbstractInterpreter,
@nospecialize(src), @nospecialize(info::CallInfo), stmt_flag::UInt32)
if isa(src, MaybeCompressed)
src_inlineable = is_stmt_inline(stmt_flag) || is_inlineable(src)
return src_inlineable ? src : nothing
return src_inlineable
elseif isa(src, IRCode)
return src
return true
elseif isa(src, SemiConcreteResult)
return src
elseif isa(src, CodeInstance)
return inlining_policy(interp, src.inferred, info, stmt_flag)
return true
end
return nothing
@assert !isa(src, CodeInstance) # handled by caller
return false
end

function inlining_policy end # deprecated legacy name used by Cthulhu

struct InliningState{Interp<:AbstractInterpreter}
edges::Vector{Any}
world::UInt
Expand Down
65 changes: 40 additions & 25 deletions base/compiler/ssair/inlining.jl
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,7 @@ function compileable_specialization(match::MethodMatch, effects::Effects,
end

struct InferredResult
src::Any
src::Any # CodeInfo or IRCode
effects::Effects
InferredResult(@nospecialize(src), effects::Effects) = new(src, effects)
end
Expand All @@ -849,11 +849,9 @@ end
# in this case function can be inlined to a constant
return ConstantCase(quoted(code.rettype_const))
end
src = @atomic :monotonic code.inferred
effects = decode_effects(code.ipo_purity_bits)
return InferredResult(src, effects)
return code
end
return InferredResult(nothing, Effects())
return nothing
end
@inline function get_local_result(inf_result::InferenceResult)
effects = inf_result.ipo_effects
Expand Down Expand Up @@ -887,7 +885,15 @@ function resolve_todo(mi::MethodInstance, result::Union{Nothing,InferenceResult,
add_inlining_backedge!(et, mi)
return inferred_result
end
(; src, effects) = inferred_result
if inferred_result isa InferredResult
(; src, effects) = inferred_result
elseif inferred_result isa CodeInstance
src = @atomic :monotonic inferred_result.inferred
effects = decode_effects(inferred_result.ipo_purity_bits)
else
src = nothing
effects = Effects()
end

# the duplicated check might have been done already within `analyze_method!`, but still
# we need it here too since we may come here directly using a constant-prop' result
Expand All @@ -896,12 +902,13 @@ function resolve_todo(mi::MethodInstance, result::Union{Nothing,InferenceResult,
compilesig_invokes=OptimizationParams(state.interp).compilesig_invokes)
end

src = inlining_policy(state.interp, src, info, flag)
src === nothing && return compileable_specialization(mi, effects, et, info;
compilesig_invokes=OptimizationParams(state.interp).compilesig_invokes)
src_inlining_policy(state.interp, src, info, flag) ||
return compileable_specialization(mi, effects, et, info;
compilesig_invokes=OptimizationParams(state.interp).compilesig_invokes)

add_inlining_backedge!(et, mi)
ir = retrieve_ir_for_inlining(mi, src, preserve_local_sources)
ir = inferred_result isa CodeInstance ? retrieve_ir_for_inlining(inferred_result, src) :
retrieve_ir_for_inlining(mi, src, preserve_local_sources)
return InliningTodo(mi, ir, effects)
end

Expand All @@ -919,14 +926,22 @@ function resolve_todo(mi::MethodInstance, @nospecialize(info::CallInfo), flag::U
add_inlining_backedge!(et, mi)
return cached_result
end
(; src, effects) = cached_result

src = inlining_policy(state.interp, src, info, flag)

src === nothing && return nothing
if cached_result isa InferredResult
(; src, effects) = cached_result
elseif cached_result isa CodeInstance
src = @atomic :monotonic cached_result.inferred
effects = decode_effects(cached_result.ipo_purity_bits)
else
src = nothing
effects = Effects()
end

preserve_local_sources = true
src_inlining_policy(state.interp, src, info, flag) || return nothing
ir = cached_result isa CodeInstance ? retrieve_ir_for_inlining(cached_result, src) :
retrieve_ir_for_inlining(mi, src, preserve_local_sources)
add_inlining_backedge!(et, mi)
return InliningTodo(mi, retrieve_ir_for_inlining(mi, src), effects)
return InliningTodo(mi, ir, effects)
end

function validate_sparams(sparams::SimpleVector)
Expand Down Expand Up @@ -979,17 +994,17 @@ function analyze_method!(match::MethodMatch, argtypes::Vector{Any},
return resolve_todo(mi, volatile_inf_result, info, flag, state; invokesig)
end

function retrieve_ir_for_inlining(mi::MethodInstance, src::String, ::Bool=true)
src = _uncompressed_ir(mi.def, src)
return inflate_ir!(src, mi)
function retrieve_ir_for_inlining(cached_result::CodeInstance, src::MaybeCompressed)

This comment has been minimized.

Copy link
@aviatesk

aviatesk Feb 26, 2024

Member

This design of forcing external abstract interpreters that use custom cache data to overload retrieve_ir_for_inlining doesn't seem ideal, as it ends up making those interpreters invalidate the base compiler pipeline seriously. The previous design where inlining_policy(interp, ...) returns data specialized for each interp would avoid this issue of invalidation.

This comment has been minimized.

Copy link
@vtjnash

vtjnash Feb 26, 2024

Author Member

Do you want to just force this to be dynamic always? There didn’t seem to be quite the right information present, since it only wants to consider if the src is maybe useful before spending the cost to expand it

src = _uncompressed_ir(cached_result, src)::CodeInfo
return inflate_ir!(src, cached_result.def)
end
function retrieve_ir_for_inlining(mi::MethodInstance, src::CodeInfo, preserve_local_sources::Bool=true)
function retrieve_ir_for_inlining(mi::MethodInstance, src::CodeInfo, preserve_local_sources::Bool)
if preserve_local_sources
src = copy(src)
end
return inflate_ir!(src, mi)
end
function retrieve_ir_for_inlining(::MethodInstance, ir::IRCode, preserve_local_sources::Bool=true)
function retrieve_ir_for_inlining(mi::MethodInstance, ir::IRCode, preserve_local_sources::Bool)
if preserve_local_sources
ir = copy(ir)
end
Expand Down Expand Up @@ -1494,13 +1509,13 @@ function semiconcrete_result_item(result::SemiConcreteResult,
return compileable_specialization(mi, result.effects, et, info;
compilesig_invokes=OptimizationParams(state.interp).compilesig_invokes)
end
ir = inlining_policy(state.interp, result.ir, info, flag)
ir === nothing && return compileable_specialization(mi, result.effects, et, info;
compilesig_invokes=OptimizationParams(state.interp).compilesig_invokes)
src_inlining_policy(state.interp, result.ir, info, flag) ||
return compileable_specialization(mi, result.effects, et, info;
compilesig_invokes=OptimizationParams(state.interp).compilesig_invokes)

add_inlining_backedge!(et, mi)
preserve_local_sources = OptimizationParams(state.interp).preserve_local_sources
ir = retrieve_ir_for_inlining(mi, ir, preserve_local_sources)
ir = retrieve_ir_for_inlining(mi, result.ir, preserve_local_sources)
return InliningTodo(mi, ir, result.effects)
end

Expand Down
7 changes: 3 additions & 4 deletions base/compiler/ssair/passes.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1492,12 +1492,11 @@ function try_inline_finalizer!(ir::IRCode, argexprs::Vector{Any}, idx::Int,
end
src = @atomic :monotonic code.inferred
else
src = nothing
return false
end

src = inlining_policy(inlining.interp, src, info, IR_FLAG_NULL)
src === nothing && return false
src = retrieve_ir_for_inlining(mi, src)
src_inlining_policy(inlining.interp, src, info, IR_FLAG_NULL) || return false
src = retrieve_ir_for_inlining(code, src)

# For now: Require finalizer to only have one basic block
length(src.cfg.blocks) == 1 || return false
Expand Down
39 changes: 20 additions & 19 deletions base/compiler/typeinfer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -310,11 +310,20 @@ function CodeInstance(interp::AbstractInterpreter, result::InferenceResult;
end
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
uncompressed = inferred_result
inferred_result = maybe_compress_codeinfo(interp, result.linfo, inferred_result, can_discard_trees)
result.is_src_volatile |= uncompressed !== inferred_result

This comment has been minimized.

Copy link
@aviatesk

aviatesk Feb 26, 2024

Member

Any particular reason for moving this out of transform_result_for_cache? Looks like transform_result_for_cache is mostly, and its signature should be transform_result_for_cache(::AbstractInterpreter, ::InferenceResult).

This comment has been minimized.

Copy link
@vtjnash

vtjnash Feb 26, 2024

Author Member

Yes, I was optimizing how compression works, but it meant it now might not return a single portable object alone representing compression of a Method, but only be possible to access it within the context of a specific CodeInfo or CodeInstance

This comment has been minimized.

Copy link
@vtjnash

vtjnash Feb 26, 2024

Author Member

In every other part of the code, compression always has the CodeInstance/CodeInfo available, so we should try to take advantage of that, which is why it needs both here also

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))
Expand All @@ -323,15 +332,21 @@ function CodeInstance(interp::AbstractInterpreter, result::InferenceResult;
relocatability = 0x1
end
end
# relocatability = isa(inferred_result, String) ? inferred_result[end] : UInt8(0)
return CodeInstance(result.linfo, cache_owner(interp),
# n.b. relocatability = (isa(inferred_result, String) && inferred_result[end]) || inferred_result === nothing
return CodeInstance(result.linfo, owner,
widenconst(result_type), widenconst(result.exc_result), rettype_const, inferred_result,
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)
end

function transform_result_for_cache(interp::AbstractInterpreter,
linfo::MethodInstance, valid_worlds::WorldRange, result::InferenceResult,
can_discard_trees::Bool=may_discard_trees(interp))
return result.src
end

function maybe_compress_codeinfo(interp::AbstractInterpreter, linfo::MethodInstance, ci::CodeInfo,
can_discard_trees::Bool=may_discard_trees(interp))
def = linfo.def
Expand All @@ -354,22 +369,6 @@ function maybe_compress_codeinfo(interp::AbstractInterpreter, linfo::MethodInsta
end
end

function transform_result_for_cache(interp::AbstractInterpreter,
linfo::MethodInstance, valid_worlds::WorldRange, result::InferenceResult,
can_discard_trees::Bool=may_discard_trees(interp))
inferred_result = result.src
if inferred_result isa CodeInfo
uncompressed = inferred_result
inferred_result = maybe_compress_codeinfo(interp, linfo, inferred_result, can_discard_trees)
result.is_src_volatile |= uncompressed !== inferred_result
end
# The global cache can only handle objects that codegen understands
if !isa(inferred_result, MaybeCompressed)
inferred_result = nothing
end
return inferred_result
end

function cache_result!(interp::AbstractInterpreter, result::InferenceResult)
if last(result.valid_worlds) == get_world_counter()
# if we've successfully recorded all of the backedges in the global reverse-cache,
Expand Down Expand Up @@ -874,7 +873,7 @@ function typeinf_edge(interp::AbstractInterpreter, method::Method, @nospecialize
exc_bestguess = refine_exception_type(frame.exc_bestguess, effects)
# propagate newly inferred source to the inliner, allowing efficient inlining w/o deserialization:
# note that this result is cached globally exclusively, we can use this local result destructively
volatile_inf_result = isinferred && (force_inline || inlining_policy(interp, result.src, NoCallInfo(), IR_FLAG_NULL) !== nothing) ?
volatile_inf_result = isinferred && (force_inline || src_inlining_policy(interp, result.src, NoCallInfo(), IR_FLAG_NULL) !== nothing) ?
VolatileInferenceResult(result) : nothing
return EdgeCallResult(frame.bestguess, exc_bestguess, edge, effects, volatile_inf_result)
elseif frame === true
Expand Down Expand Up @@ -1092,6 +1091,8 @@ function ci_meets_requirement(code::CodeInstance, source_mode::UInt8, ci_is_cach
return false
end

_uncompressed_ir(ci::Core.CodeInstance, s::String) = ccall(:jl_uncompress_ir, Any, (Any, Any, Any), ci.def.def::Method, ci, s)::CodeInfo

# compute (and cache) an inferred AST and return type
function typeinf_ext(interp::AbstractInterpreter, mi::MethodInstance, source_mode::UInt8)
start_time = ccall(:jl_typeinf_timing_begin, UInt64, ())
Expand Down
2 changes: 1 addition & 1 deletion base/compiler/utilities.jl
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ function retrieve_code_info(linfo::MethodInstance, world::UInt)
# can happen in images built with --strip-ir
return nothing
elseif isa(src, String)
c = _uncompressed_ir(def, src)
c = ccall(:jl_uncompress_ir, Any, (Any, Ptr{Cvoid}, Any), def, C_NULL, src)
else
c = copy(src::CodeInfo)
end
Expand Down
12 changes: 7 additions & 5 deletions base/reflection.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1313,12 +1313,14 @@ function length(mt::Core.MethodTable)
end
isempty(mt::Core.MethodTable) = (mt.defs === nothing)

uncompressed_ir(m::Method) = isdefined(m, :source) ? _uncompressed_ir(m, m.source) :
uncompressed_ir(m::Method) = isdefined(m, :source) ? _uncompressed_ir(m) :
isdefined(m, :generator) ? error("Method is @generated; try `code_lowered` instead.") :
error("Code for this Method is not available.")
_uncompressed_ir(m::Method, s::CodeInfo) = copy(s)
_uncompressed_ir(m::Method, s::String) = ccall(:jl_uncompress_ir, Any, (Any, Ptr{Cvoid}, Any), m, C_NULL, s)::CodeInfo
_uncompressed_ir(ci::Core.CodeInstance, s::String) = ccall(:jl_uncompress_ir, Any, (Any, Any, Any), ci.def.def::Method, ci, s)::CodeInfo
function _uncompressed_ir(m::Method)
s = m.source
s isa String && (s = ccall(:jl_uncompress_ir, Any, (Any, Ptr{Cvoid}, Any), m, C_NULL, s))
return s::CodeInfo
end
# for backwards compat
const uncompressed_ast = uncompressed_ir
const _uncompressed_ast = _uncompressed_ir
Expand Down Expand Up @@ -1632,7 +1634,7 @@ function get_oc_code_rt(@nospecialize(oc::Core.OpaqueClosure))
ccall(:jl_is_in_pure_context, Bool, ()) && error("code reflection cannot be used from generated functions")
m = oc.source
if isa(m, Method)
code = _uncompressed_ir(m, m.source)
code = _uncompressed_ir(m)
return Pair{CodeInfo,Any}(code, typeof(oc).parameters[2])
else
error("encountered invalid Core.OpaqueClosure object")
Expand Down
4 changes: 2 additions & 2 deletions stdlib/Serialization/src/Serialization.jl
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ Provide serialization of Julia objects via the functions
"""
module Serialization

import Base: GMP, Bottom, unsafe_convert, uncompressed_ast
import Base: GMP, Bottom, unsafe_convert
import Core: svec, SimpleVector
using Base: unaliascopy, unwrap_unionall, require_one_based_indexing, ntupleany
using Core.IR
Expand Down Expand Up @@ -447,7 +447,7 @@ function serialize(s::AbstractSerializer, meth::Method)
serialize(s, meth.constprop)
serialize(s, meth.purity)
if isdefined(meth, :source)
serialize(s, Base._uncompressed_ast(meth, meth.source))
serialize(s, Base._uncompressed_ast(meth))
else
serialize(s, nothing)
end
Expand Down
16 changes: 10 additions & 6 deletions test/compiler/AbstractInterpreter.jl
Original file line number Diff line number Diff line change
Expand Up @@ -334,12 +334,12 @@ function CC.abstract_call(interp::NoinlineInterpreter,
end
return ret
end
function CC.inlining_policy(interp::NoinlineInterpreter,
function CC.src_inlining_policy(interp::NoinlineInterpreter,
@nospecialize(src), @nospecialize(info::CallInfo), stmt_flag::UInt32)
if isa(info, NoinlineCallInfo)
return nothing
return false
end
return @invoke CC.inlining_policy(interp::CC.AbstractInterpreter,
return @invoke CC.src_inlining_policy(interp::CC.AbstractInterpreter,
src::Any, info::CallInfo, stmt_flag::UInt32)
end

Expand Down Expand Up @@ -485,14 +485,18 @@ function CC.transform_result_for_cache(interp::CustomDataInterp,
mi::Core.MethodInstance, valid_worlds::CC.WorldRange, result::CC.InferenceResult)
return CustomData(inferred_result)
end
function CC.inlining_policy(interp::CustomDataInterp, @nospecialize(src),
function CC.src_inlining_policy(interp::CustomDataInterp, @nospecialize(src),
@nospecialize(info::CC.CallInfo), stmt_flag::UInt32)
if src isa CustomData
src = src.inferred
end
return @invoke CC.inlining_policy(interp::CC.AbstractInterpreter, src::Any,
info::CC.CallInfo, stmt_flag::UInt32)
return @invoke CC.src_inlining_policy(interp::CC.AbstractInterpreter, src::Any,
info::CC.CallInfo, stmt_flag::UInt32)
end
CC.retrieve_ir_for_inlining(cached_result::CodeInstance, src::CustomData) =
CC.retrieve_ir_for_inlining(cached_result, src.inferred)
CC.retrieve_ir_for_inlining(mi::MethodInstance, src::CustomData, preserve_local_sources::Bool) =
CC.retrieve_ir_for_inlining(mi, src.inferred, preserve_local_sources)
let src = code_typed((Int,); interp=CustomDataInterp()) do x
return sin(x) + cos(x)
end |> only |> first
Expand Down
10 changes: 7 additions & 3 deletions test/precompile.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1794,14 +1794,18 @@ let newinterp_path = abspath("compiler/newinterp.jl")
mi::Core.MethodInstance, valid_worlds::CC.WorldRange, result::CC.InferenceResult)
return CustomData(inferred_result)
end
function CC.inlining_policy(interp::PrecompileInterpreter, @nospecialize(src),
function CC.src_inlining_policy(interp::PrecompileInterpreter, @nospecialize(src),
@nospecialize(info::CC.CallInfo), stmt_flag::UInt32)
if src isa CustomData
src = src.inferred
end
return @invoke CC.inlining_policy(interp::CC.AbstractInterpreter, src::Any,
info::CC.CallInfo, stmt_flag::UInt32)
return @invoke CC.src_inlining_policy(interp::CC.AbstractInterpreter, src::Any,
info::CC.CallInfo, stmt_flag::UInt32)
end
CC.retrieve_ir_for_inlining(cached_result::Core.CodeInstance, src::CustomData) =
CC.retrieve_ir_for_inlining(cached_result, src.inferred)
CC.retrieve_ir_for_inlining(mi::Core.MethodInstance, src::CustomData, preserve_local_sources::Bool) =
CC.retrieve_ir_for_inlining(mi, src.inferred, preserve_local_sources)
end

Base.return_types((Float64,)) do x
Expand Down

0 comments on commit 7057763

Please sign in to comment.