Skip to content

Commit

Permalink
inlining: Don't inline concrete-eval'ed calls whose result was too large
Browse files Browse the repository at this point in the history
This undoes some of the choices made in #47283 and #47305.
As Shuhei correctly pointed out, even with the restriction to
`nothrow`, adding the extra flags on the inlined statements
results in incorrect IR. Also, my bigger motivating test case
turns out to be insufficiently optimized without the effect_free
flags (which I removed in the final revision of #47305).
I think for the time being, the best course of action here
is to just stop inlining concrete-eval'ed
calls entirely. The const result is available for inference,
so in most cases the call will get deleted. If there's an
important case we care about where this does not happen,
we should take a look at that separately.
  • Loading branch information
Keno committed Oct 28, 2022
1 parent f71b839 commit aac7786
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 94 deletions.
99 changes: 25 additions & 74 deletions base/compiler/ssair/inlining.jl
Original file line number Diff line number Diff line change
Expand Up @@ -370,8 +370,7 @@ 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}},
extra_flags::UInt8 = inlined_flags_for_effects(item.effects))
boundscheck::Symbol, todo_bbs::Vector{Tuple{Int, Int}})
# Ok, do the inlining here
sparam_vals = item.mi.sparam_vals
def = item.mi.def::Method
Expand Down Expand Up @@ -412,7 +411,6 @@ function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector
break
end
inline_compact[idx′] = stmt′
inline_compact[SSAValue(idx′)][:flag] |= extra_flags
end
just_fixup!(inline_compact, new_new_offset, late_fixup_offset)
compact.result_idx = inline_compact.result_idx
Expand Down Expand Up @@ -447,14 +445,6 @@ function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector
stmt′ = PhiNode(Int32[edge+bb_offset for edge in stmt′.edges], stmt′.values)
end
inline_compact[idx′] = stmt′
if extra_flags != 0 && !isa(stmt′, Union{GotoNode, GotoIfNot})
if (extra_flags & IR_FLAG_NOTHROW) != 0 && inline_compact[SSAValue(idx′)][:type] === Union{}
# Shown nothrow, but also guaranteed to throw => unreachable
inline_compact[idx′] = ReturnNode()
else
inline_compact[SSAValue(idx′)][:flag] |= extra_flags
end
end
end
just_fixup!(inline_compact, new_new_offset, late_fixup_offset)
compact.result_idx = inline_compact.result_idx
Expand Down Expand Up @@ -1012,37 +1002,6 @@ function flags_for_effects(effects::Effects)
return flags
end

"""
inlined_flags_for_effects(effects::Effects)
This function answers the query:
Given a call site annotated as `effects`, what can we say about each inlined
statement after the inlining?
Note that this is different from `flags_for_effects`, which just talks about
the call site itself. Consider for example:
````
function foo()
V = Any[]
push!(V, 1)
tuple(V...)
end
```
This function is properly inferred effect_free, because it has no global effects.
However, we may not inline each statement with an :effect_free flag, because
that would incorrectly lose the `push!`.
"""
function inlined_flags_for_effects(effects::Effects)
flags::UInt8 = 0
if is_nothrow(effects)
flags |= IR_FLAG_NOTHROW
end
return flags
end

function handle_single_case!(todo::Vector{Pair{Int,Any}},
ir::IRCode, idx::Int, stmt::Expr, @nospecialize(case), params::OptimizationParams,
isinvoke::Bool = false)
Expand Down Expand Up @@ -1221,24 +1180,20 @@ function handle_invoke_call!(todo::Vector{Pair{Int,Any}},
invokesig = sig.argtypes
override_effects = EFFECTS_UNKNOWN′
if isa(result, ConcreteResult)
if may_inline_concrete_result(result)
item = concrete_result_item(result, state; invokesig)
handle_single_case!(todo, ir, idx, stmt, item, state.params, true)
return nothing
end
override_effects = result.effects
end
argtypes = invoke_rewrite(sig.argtypes)
if isa(result, ConstPropResult)
mi = result.result.linfo
validate_sparams(mi.sparam_vals) || return nothing
if argtypes_to_type(argtypes) <: mi.def.sig
item = resolve_todo(mi, result.result, argtypes, info, flag, state; invokesig, override_effects)
handle_single_case!(todo, ir, idx, stmt, item, state.params, true)
return nothing
item = concrete_result_item(result, state, info; invokesig)
else
argtypes = invoke_rewrite(sig.argtypes)
if isa(result, ConstPropResult)
mi = result.result.linfo
validate_sparams(mi.sparam_vals) || return nothing
if argtypes_to_type(argtypes) <: mi.def.sig
item = resolve_todo(mi, result.result, argtypes, info, flag, state; invokesig, override_effects)
handle_single_case!(todo, ir, idx, stmt, item, state.params, true)
return nothing
end
end
item = analyze_method!(match, argtypes, info, flag, state; allow_typevars=false, invokesig, override_effects)
end
item = analyze_method!(match, argtypes, info, flag, state; allow_typevars=false, invokesig, override_effects)
handle_single_case!(todo, ir, idx, stmt, item, state.params, true)
return nothing
end
Expand Down Expand Up @@ -1352,12 +1307,7 @@ function handle_any_const_result!(cases::Vector{InliningCase},
allow_abstract::Bool, allow_typevars::Bool)
override_effects = EFFECTS_UNKNOWN′
if isa(result, ConcreteResult)
if may_inline_concrete_result(result)
return handle_concrete_result!(cases, result, state)
else
override_effects = result.effects
result = nothing
end
return handle_concrete_result!(cases, result, state, info)
end
if isa(result, SemiConcreteResult)
result = inlining_policy(state.interp, result, info, flag, result.mi, argtypes)
Expand Down Expand Up @@ -1538,18 +1488,24 @@ function handle_semi_concrete_result!(cases::Vector{InliningCase}, result::SemiC
return true
end

function handle_concrete_result!(cases::Vector{InliningCase}, result::ConcreteResult, state::InliningState)
case = concrete_result_item(result, state)
function handle_concrete_result!(cases::Vector{InliningCase}, result::ConcreteResult, state::InliningState, @nospecialize(info::CallInfo))
case = concrete_result_item(result, state, info)
push!(cases, InliningCase(result.mi.specTypes, case))
return true
end

may_inline_concrete_result(result::ConcreteResult) =
isdefined(result, :result) && is_inlineable_constant(result.result)

function concrete_result_item(result::ConcreteResult, state::InliningState;
function concrete_result_item(result::ConcreteResult, state::InliningState, @nospecialize(info::CallInfo);
invokesig::Union{Nothing,Vector{Any}}=nothing)
@assert may_inline_concrete_result(result)
if !may_inline_concrete_result(result)
et = InliningEdgeTracker(state.et, invokesig)
case = compileable_specialization(result.mi, result.effects, et, info;
compilesig_invokes=state.params.compilesig_invokes)
@assert case !== nothing "concrete evaluation should never happen for uncompileable callsite"
return case
end
@assert result.effects === EFFECTS_TOTAL
return ConstantCase(quoted(result.result))
end
Expand Down Expand Up @@ -1583,12 +1539,7 @@ function handle_opaque_closure_call!(todo::Vector{Pair{Int,Any}},
validate_sparams(mi.sparam_vals) || return nothing
item = resolve_todo(mi, result.result, sig.argtypes, info, flag, state)
elseif isa(result, ConcreteResult)
if may_inline_concrete_result(result)
item = concrete_result_item(result, state)
else
override_effects = result.effects
item = analyze_method!(info.match, sig.argtypes, info, flag, state; allow_typevars=false, override_effects)
end
item = concrete_result_item(result, state, info)
else
item = analyze_method!(info.match, sig.argtypes, info, flag, state; allow_typevars=false)
end
Expand Down
20 changes: 0 additions & 20 deletions test/compiler/inline.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1039,26 +1039,6 @@ struct FooTheRef
x::Ref
FooTheRef(v) = new(v === nothing ? THE_REF_NULL : THE_REF)
end
let src = code_typed1() do
FooTheRef(nothing)
end
@test count(isnew, src.code) == 1
end
let src = code_typed1() do
FooTheRef(0)
end
@test count(isnew, src.code) == 1
end
let src = code_typed1() do
@invoke FooTheRef(nothing::Any)
end
@test count(isnew, src.code) == 1
end
let src = code_typed1() do
@invoke FooTheRef(0::Any)
end
@test count(isnew, src.code) == 1
end
@test fully_eliminated() do
FooTheRef(nothing)
nothing
Expand Down

0 comments on commit aac7786

Please sign in to comment.