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

Eager finalizer insertion #45272

Merged
merged 4 commits into from
Jun 7, 2022
Merged

Eager finalizer insertion #45272

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da04e84
Eager finalizer insertion
Keno May 11, 2022
c39f046
Merge branch 'master' into kf/eagerfinalizers
aviatesk May 30, 2022
030be84
rm redundant copy
Keno May 30, 2022
cfc2abb
Merge branch 'master' into kf/eagerfinalizers
Keno Jun 6, 2022
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
11 changes: 11 additions & 0 deletions base/compiler/abstractinterpretation.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1590,6 +1590,15 @@ function invoke_rewrite(xs::Vector{Any})
return newxs
end

function abstract_finalizer(interp::AbstractInterpreter, argtypes::Vector{Any}, sv::InferenceState)
if length(argtypes) == 3
finalizer_argvec = Any[argtypes[2], argtypes[3]]
call = abstract_call(interp, ArgInfo(nothing, finalizer_argvec), sv, 1)
return CallMeta(Nothing, Effects(), FinalizerInfo(call.info, call.effects))
Keno marked this conversation as resolved.
Show resolved Hide resolved
end
return CallMeta(Nothing, Effects(), false)
end

# call where the function is known exactly
function abstract_call_known(interp::AbstractInterpreter, @nospecialize(f),
arginfo::ArgInfo, sv::InferenceState,
Expand All @@ -1604,6 +1613,8 @@ function abstract_call_known(interp::AbstractInterpreter, @nospecialize(f),
return abstract_invoke(interp, arginfo, sv)
elseif f === modifyfield!
return abstract_modifyfield!(interp, argtypes, sv)
elseif f === Core.finalizer
return abstract_finalizer(interp, argtypes, sv)
end
rt = abstract_call_builtin(interp, f, arginfo, sv, max_methods)
return CallMeta(rt, builtin_effects(f, argtypes, rt), false)
Expand Down
5 changes: 4 additions & 1 deletion base/compiler/optimize.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ const IR_FLAG_THROW_BLOCK = 0x01 << 3
# This statement may be removed if its result is unused. In particular it must
# thus be both pure and effect free.
const IR_FLAG_EFFECT_FREE = 0x01 << 4
# This statement was proven not to throw
const IR_FLAG_NOTHROW = 0x01 << 5


const TOP_TUPLE = GlobalRef(Core, :tuple)

Expand Down Expand Up @@ -567,7 +570,7 @@ function run_passes(
@pass "Inlining" ir = ssa_inlining_pass!(ir, ir.linetable, sv.inlining, ci.propagate_inbounds)
# @timeit "verify 2" verify_ir(ir)
@pass "compact 2" ir = compact!(ir)
@pass "SROA" ir = sroa_pass!(ir)
@pass "SROA" ir = sroa_pass!(ir, sv.inlining)
@pass "ADCE" ir = adce_pass!(ir)
@pass "type lift" ir = type_lift_pass!(ir)
@pass "compact 3" ir = compact!(ir)
Expand Down
134 changes: 98 additions & 36 deletions base/compiler/ssair/inlining.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,21 +308,17 @@ function finish_cfg_inline!(state::CFGInliningState)
end
end

function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector{Any},
linetable::Vector{LineInfoNode}, item::InliningTodo,
boundscheck::Symbol, todo_bbs::Vector{Tuple{Int, Int}})
# Ok, do the inlining here
spec = item.spec::ResolvedInliningSpec
sparam_vals = item.mi.sparam_vals
def = item.mi.def::Method
function ir_inline_linetable!(linetable::Vector{LineInfoNode}, inlinee_ir::IRCode,
inlinee::Method,
inlined_at::Int32)
coverage = coverage_enabled(inlinee.module)
linetable_offset::Int32 = length(linetable)
# Append the linetable of the inlined function to our line table
inlined_at = compact.result[idx][:line]
topline::Int32 = linetable_offset + Int32(1)
coverage = coverage_enabled(def.module)
coverage_by_path = JLOptions().code_coverage == 3
push!(linetable, LineInfoNode(def.module, def.name, def.file, def.line, inlined_at))
oldlinetable = spec.ir.linetable
push!(linetable, LineInfoNode(inlinee.module, inlinee.name, inlinee.file, inlinee.line, inlined_at))
oldlinetable = inlinee_ir.linetable
extra_coverage_line = 0
for oldline in 1:length(oldlinetable)
entry = oldlinetable[oldline]
if !coverage && coverage_by_path && is_file_tracked(entry.file)
Expand All @@ -341,8 +337,25 @@ function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector
end
push!(linetable, newentry)
end
if coverage && spec.ir.stmts[1][:line] + linetable_offset != topline
insert_node_here!(compact, NewInstruction(Expr(:code_coverage_effect), Nothing, topline))
if coverage && inlinee_ir.stmts[1][:line] + linetable_offset != topline
extra_coverage_line = topline
end
return linetable_offset, extra_coverage_line
end

function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector{Any},
linetable::Vector{LineInfoNode}, item::InliningTodo,
boundscheck::Symbol, todo_bbs::Vector{Tuple{Int, Int}})
# Ok, do the inlining here
spec = item.spec::ResolvedInliningSpec
sparam_vals = item.mi.sparam_vals
def = item.mi.def::Method
inlined_at = compact.result[idx][:line]
linetable_offset::Int32 = length(linetable)
topline::Int32 = linetable_offset + Int32(1)
linetable_offset, extra_coverage_line = ir_inline_linetable!(linetable, item.spec.ir, def, inlined_at)
if extra_coverage_line != 0
insert_node_here!(compact, NewInstruction(Expr(:code_coverage_effect), Nothing, extra_coverage_line))
end
if def.isva
nargs_def = Int(def.nargs::Int32)
Expand Down Expand Up @@ -839,7 +852,7 @@ function resolve_todo(todo::InliningTodo, state::InliningState, flag::UInt8)
src === nothing && return compileable_specialization(et, match, effects)

et !== nothing && push!(et, mi)
return InliningTodo(mi, src, effects)
return InliningTodo(mi, retrieve_ir_for_inlining(mi, src), effects)
end

function resolve_todo((; fully_covered, atype, cases, #=bbs=#)::UnionSplit, state::InliningState, flag::UInt8)
Expand All @@ -861,7 +874,8 @@ function validate_sparams(sparams::SimpleVector)
end

function analyze_method!(match::MethodMatch, argtypes::Vector{Any},
flag::UInt8, state::InliningState)
flag::UInt8, state::InliningState,
do_resolve::Bool = true)
method = match.method
spec_types = match.spec_types

Expand Down Expand Up @@ -895,26 +909,20 @@ function analyze_method!(match::MethodMatch, argtypes::Vector{Any},
todo = InliningTodo(mi, match, argtypes)
# If we don't have caches here, delay resolving this MethodInstance
# until the batch inlining step (or an external post-processing pass)
state.mi_cache === nothing && return todo
do_resolve && state.mi_cache === nothing && return todo
return resolve_todo(todo, state, flag)
end

function InliningTodo(mi::MethodInstance, ir::IRCode, effects::Effects)
ir = copy(ir)
Copy link
Member

Choose a reason for hiding this comment

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

Should this have been a separate PR? Seems like an odd thing to have in the commit without any description except "redundant"

Copy link
Member Author

Choose a reason for hiding this comment

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

This copy was moved somewhere else in a conflicting PR while this PR was pending, so after a rebase there ended up being a duplicated copy call that wasn't in the original PR

Copy link
Member

Choose a reason for hiding this comment

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

Why is it in the commit message?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oversight during the squash merge.

Copy link
Member

Choose a reason for hiding this comment

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

ah, okay

return InliningTodo(mi, ResolvedInliningSpec(ir, effects))
end

function InliningTodo(mi::MethodInstance, src::Union{CodeInfo, Vector{UInt8}}, effects::Effects)
if !isa(src, CodeInfo)
src = ccall(:jl_uncompress_ir, Any, (Any, Ptr{Cvoid}, Any), mi.def, C_NULL, src::Vector{UInt8})::CodeInfo
else
src = copy(src)
end
@timeit "inline IR inflation" begin
ir = inflate_ir!(src, mi)::IRCode
return InliningTodo(mi, ResolvedInliningSpec(ir, effects))
end
function retrieve_ir_for_inlining(mi::MethodInstance, src::Array{UInt8, 1})
src = ccall(:jl_uncompress_ir, Any, (Any, Ptr{Cvoid}, Any), mi.def, C_NULL, src::Vector{UInt8})::CodeInfo
return inflate_ir!(src, mi)
end
retrieve_ir_for_inlining(mi::MethodInstance, src::CodeInfo) = inflate_ir(src, mi)::IRCode
retrieve_ir_for_inlining(mi::MethodInstance, ir::IRCode) = copy(ir)

function handle_single_case!(
ir::IRCode, idx::Int, stmt::Expr,
Expand Down Expand Up @@ -1196,7 +1204,7 @@ function process_simple!(ir::IRCode, idx::Int, state::InliningState, todo::Vecto
end
end

if sig.f !== Core.invoke && is_builtin(sig)
if sig.f !== Core.invoke && sig.f !== Core.finalizer && is_builtin(sig)
# No inlining for builtins (other invoke/apply/typeassert)
return nothing
end
Expand All @@ -1213,9 +1221,10 @@ function process_simple!(ir::IRCode, idx::Int, state::InliningState, todo::Vecto
end

# TODO inline non-`isdispatchtuple`, union-split callsites?
function analyze_single_call!(
ir::IRCode, idx::Int, stmt::Expr, infos::Vector{MethodMatchInfo}, flag::UInt8,
sig::Signature, state::InliningState, todo::Vector{Pair{Int, Any}})
function compute_inlining_cases(
infos::Vector{MethodMatchInfo}, flag::UInt8,
sig::Signature, state::InliningState,
do_resolve::Bool = true)
argtypes = sig.argtypes
cases = InliningCase[]
local any_fully_covered = false
Expand All @@ -1232,7 +1241,7 @@ function analyze_single_call!(
continue
end
for match in meth
handled_all_cases &= handle_match!(match, argtypes, flag, state, cases, true)
handled_all_cases &= handle_match!(match, argtypes, flag, state, cases, true, do_resolve)
any_fully_covered |= match.fully_covers
end
end
Expand All @@ -1242,8 +1251,18 @@ function analyze_single_call!(
filter!(case::InliningCase->isdispatchtuple(case.sig), cases)
end

handle_cases!(ir, idx, stmt, argtypes_to_type(argtypes), cases,
handled_all_cases & any_fully_covered, todo, state.params)
return cases, handled_all_cases & any_fully_covered
end

function analyze_single_call!(
ir::IRCode, idx::Int, stmt::Expr, infos::Vector{MethodMatchInfo}, flag::UInt8,
sig::Signature, state::InliningState, todo::Vector{Pair{Int, Any}})

r = compute_inlining_cases(infos, flag, sig, state)
r === nothing && return nothing
cases, all_covered = r
handle_cases!(ir, idx, stmt, argtypes_to_type(sig.argtypes), cases,
all_covered, todo, state.params)
end

# similar to `analyze_single_call!`, but with constant results
Expand Down Expand Up @@ -1295,14 +1314,15 @@ end

function handle_match!(
match::MethodMatch, argtypes::Vector{Any}, flag::UInt8, state::InliningState,
cases::Vector{InliningCase}, allow_abstract::Bool = false)
cases::Vector{InliningCase}, allow_abstract::Bool = false,
do_resolve::Bool = true)
spec_types = match.spec_types
allow_abstract || isdispatchtuple(spec_types) || return false
# we may see duplicated dispatch signatures here when a signature gets widened
# during abstract interpretation: for the purpose of inlining, we can just skip
# processing this dispatch candidate
_any(case->case.sig === spec_types, cases) && return true
item = analyze_method!(match, argtypes, flag, state)
item = analyze_method!(match, argtypes, flag, state, do_resolve)
item === nothing && return false
push!(cases, InliningCase(spec_types, item))
return true
Expand Down Expand Up @@ -1417,6 +1437,48 @@ function assemble_inline_todo!(ir::IRCode, state::InliningState)
continue
end

# Handle finalizer
if sig.f === Core.finalizer
if isa(info, FinalizerInfo)
# Only inline finalizers that are known nothrow and notls.
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have this restriction? Would it be invalid to inline finalizers and have them run in the original task environment?

Obligatory GPU context: our finalizers access the TLS to inspect the current stream. On finalizer tasks, there's no such stream, so we perform a blocking free, whereas on 'regular' tasks the memory operation can be ordered against other operations on that stream. I had assumed that inlining finalizers would additionally get them to use the local stream, promoting blocking frees to asynchronously-ordered ones. Maybe that's too magical though.

Copy link
Member Author

@Keno Keno May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It's to prevent things like #42752 where finalizers unexpectedly mess with task local state. I think we're now conceptually moving into the direction of finalizers having their own pseudo-task (maybe even a real task eventually), so inlining a finalizer that looks at task state would not be legal since it changes the task environment.

That said, for your case, you want to allow that to happen, even though it's not technically semantically legal. I think for that case, we could just put a notls effect override on the finalizer.

# This avoids having to set up state for finalizer isolation
(is_nothrow(info.effects) && is_notaskstate(info.effects)) || continue

info = info.info
if isa(info, MethodMatchInfo)
infos = MethodMatchInfo[info]
elseif isa(info, UnionSplitInfo)
infos = info.matches
else
continue
end

ft = argextype(stmt.args[2], ir)
has_free_typevars(ft) && return nothing
f = singleton_type(ft)
argtypes = Vector{Any}(undef, 2)
argtypes[1] = ft
argtypes[2] = argextype(stmt.args[3], ir)
sig = Signature(f, ft, argtypes)

cases, all_covered = compute_inlining_cases(infos, UInt8(0), sig, state, false)
length(cases) == 0 && continue
if all_covered && length(cases) == 1
if isa(cases[1], InliningCase)
case1 = cases[1].item
if isa(case1, InliningTodo)
push!(stmt.args, true)
push!(stmt.args, case1.mi)
elseif isa(case1, InvokeCase)
push!(stmt.args, false)
push!(stmt.args, case1.invoke)
end
end
end
continue
end
end

# if inference arrived here with constant-prop'ed result(s),
# we can perform a specialized analysis for just this case
if isa(info, ConstCallInfo)
Expand Down
60 changes: 30 additions & 30 deletions base/compiler/ssair/ir.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,36 +163,6 @@ const AnySSAValue = Union{SSAValue, OldSSAValue, NewSSAValue}


# SSA-indexed nodes

struct NewInstruction
stmt::Any
type::Any
info::Any
# If nothing, copy the line from previous statement
# in the insertion location
line::Union{Int32, Nothing}
flag::UInt8

## Insertion options

# The IR_FLAG_EFFECT_FREE flag has already been computed (or forced).
# Don't bother redoing so on insertion.
effect_free_computed::Bool
NewInstruction(@nospecialize(stmt), @nospecialize(type), @nospecialize(info),
line::Union{Int32, Nothing}, flag::UInt8, effect_free_computed::Bool) =
new(stmt, type, info, line, flag, effect_free_computed)
end
NewInstruction(@nospecialize(stmt), @nospecialize(type)) =
NewInstruction(stmt, type, nothing)
NewInstruction(@nospecialize(stmt), @nospecialize(type), line::Union{Nothing, Int32}) =
NewInstruction(stmt, type, nothing, line, IR_FLAG_NULL, false)

effect_free(inst::NewInstruction) =
NewInstruction(inst.stmt, inst.type, inst.info, inst.line, inst.flag | IR_FLAG_EFFECT_FREE, true)
non_effect_free(inst::NewInstruction) =
NewInstruction(inst.stmt, inst.type, inst.info, inst.line, inst.flag & ~IR_FLAG_EFFECT_FREE, true)


struct InstructionStream
inst::Vector{Any}
type::Vector{Any}
Expand Down Expand Up @@ -292,6 +262,36 @@ function add!(new::NewNodeStream, pos::Int, attach_after::Bool)
end
copy(nns::NewNodeStream) = NewNodeStream(copy(nns.stmts), copy(nns.info))

struct NewInstruction
stmt::Any
type::Any
info::Any
# If nothing, copy the line from previous statement
# in the insertion location
line::Union{Int32, Nothing}
flag::UInt8

## Insertion options

# The IR_FLAG_EFFECT_FREE flag has already been computed (or forced).
# Don't bother redoing so on insertion.
effect_free_computed::Bool
NewInstruction(@nospecialize(stmt), @nospecialize(type), @nospecialize(info),
line::Union{Int32, Nothing}, flag::UInt8, effect_free_computed::Bool) =
new(stmt, type, info, line, flag, effect_free_computed)
end
NewInstruction(@nospecialize(stmt), @nospecialize(type)) =
NewInstruction(stmt, type, nothing)
NewInstruction(@nospecialize(stmt), @nospecialize(type), line::Union{Nothing, Int32}) =
NewInstruction(stmt, type, nothing, line, IR_FLAG_NULL, false)
NewInstruction(@nospecialize(stmt), meta::Instruction; line::Union{Int32, Nothing}=nothing) =
NewInstruction(stmt, meta[:type], meta[:info], line === nothing ? meta[:line] : line, meta[:flag], true)

effect_free(inst::NewInstruction) =
NewInstruction(inst.stmt, inst.type, inst.info, inst.line, inst.flag | IR_FLAG_EFFECT_FREE, true)
non_effect_free(inst::NewInstruction) =
NewInstruction(inst.stmt, inst.type, inst.info, inst.line, inst.flag & ~IR_FLAG_EFFECT_FREE, true)

struct IRCode
stmts::InstructionStream
argtypes::Vector{Any}
Expand Down
Loading