From 71e77fdf3fc7bbdc2f23d6db3cfccab502501218 Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Mon, 24 Oct 2022 11:09:02 +0000 Subject: [PATCH] inlining: Use concrete-eval effects if available It is possible for concrete-eval to refine effects, but be unable to inline (because the constant is too large, c.f. #47283). However, in that case, we would still like to use the extra effect information to make sure that the optimizer can delete the statement if it turns out to be unused. There are two cases: the first is where the call is not inlineable at all. This one is simple, because we just apply the effects on the :invoke as we usually would. The second is trickier: If we do end up inlining the call, we need to apply the overriden effects to every inlined statement, because we lose the identity of the function as a whole. This is a bit nasty and I don't really like it, but I'm not sure what a better alternative would be. We could always refuse to inline calls with large-constant results (since we currently pessimize what is being inlined anyway), but I'm not sure that would be better. This is a simple solution and works for the case I have in practice, but we may want to revisit it in the future. --- base/compiler/ssair/inlining.jl | 109 +++++++++++++++++++++++++------- test/compiler/inline.jl | 25 ++++++++ 2 files changed, 110 insertions(+), 24 deletions(-) diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl index 9a15d2dd4350f..5829c96c9d0ee 100644 --- a/base/compiler/ssair/inlining.jl +++ b/base/compiler/ssair/inlining.jl @@ -370,7 +370,8 @@ 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}}) + boundscheck::Symbol, todo_bbs::Vector{Tuple{Int, Int}}, + extra_flags::UInt8 = inlined_flags_for_effects(item.effects)) # Ok, do the inlining here sparam_vals = item.mi.sparam_vals def = item.mi.def::Method @@ -411,6 +412,7 @@ 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 @@ -445,6 +447,14 @@ 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 @@ -838,8 +848,9 @@ end # the general resolver for usual and const-prop'ed calls function resolve_todo(mi::MethodInstance, result::Union{MethodMatch,InferenceResult}, - argtypes::Vector{Any}, @nospecialize(info::CallInfo), flag::UInt8, - state::InliningState; invokesig::Union{Nothing,Vector{Any}}=nothing) + argtypes::Vector{Any}, @nospecialize(info::CallInfo), flag::UInt8, + state::InliningState; invokesig::Union{Nothing,Vector{Any}}=nothing, + override_effects::Effects = EFFECTS_UNKNOWN′) et = InliningEdgeTracker(state.et, invokesig) #XXX: update_valid_age!(min_valid[1], max_valid[1], sv) @@ -860,6 +871,10 @@ function resolve_todo(mi::MethodInstance, result::Union{MethodMatch,InferenceRes (; src, effects) = cached_result end + if override_effects !== EFFECTS_UNKNOWN′ + effects = override_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 if !state.params.inlining || is_stmt_noinline(flag) @@ -937,7 +952,8 @@ can_inline_typevars(m::MethodMatch, argtypes::Vector{Any}) = can_inline_typevars function analyze_method!(match::MethodMatch, argtypes::Vector{Any}, @nospecialize(info::CallInfo), flag::UInt8, state::InliningState; - allow_typevars::Bool, invokesig::Union{Nothing,Vector{Any}}=nothing) + allow_typevars::Bool, invokesig::Union{Nothing,Vector{Any}}=nothing, + override_effects::Effects=EFFECTS_UNKNOWN′) method = match.method spec_types = match.spec_types @@ -967,11 +983,13 @@ function analyze_method!(match::MethodMatch, argtypes::Vector{Any}, mi = specialize_method(match; preexisting=true) # Union{Nothing, MethodInstance} if mi === nothing et = InliningEdgeTracker(state.et, invokesig) - return compileable_specialization(match, Effects(), et, info; + effects = override_effects + effects === EFFECTS_UNKNOWN′ && (effects = info_effects(nothing, match, state)) + return compileable_specialization(match, effects, et, info; compilesig_invokes=state.params.compilesig_invokes) end - return resolve_todo(mi, match, argtypes, info, flag, state; invokesig) + return resolve_todo(mi, match, argtypes, info, flag, state; invokesig, override_effects) end function retrieve_ir_for_inlining(mi::MethodInstance, src::Array{UInt8, 1}) @@ -994,6 +1012,37 @@ 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) @@ -1170,21 +1219,26 @@ function handle_invoke_call!(todo::Vector{Pair{Int,Any}}, end result = info.result invokesig = sig.argtypes - if isa(result, ConcreteResult) && may_inline_concrete_result(result) - item = concrete_result_item(result, state; 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) - handle_single_case!(todo, ir, idx, stmt, item, state.params, true) - return nothing - end + 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 - item = analyze_method!(match, argtypes, info, flag, state; allow_typevars=false, invokesig) + 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 + end + 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 @@ -1296,10 +1350,12 @@ function handle_any_const_result!(cases::Vector{InliningCase}, @nospecialize(result), match::MethodMatch, argtypes::Vector{Any}, @nospecialize(info::CallInfo), flag::UInt8, state::InliningState; 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 end @@ -1313,7 +1369,7 @@ function handle_any_const_result!(cases::Vector{InliningCase}, return handle_const_prop_result!(cases, result, argtypes, info, flag, state; allow_abstract, allow_typevars) else @assert result === nothing - return handle_match!(cases, match, argtypes, info, flag, state; allow_abstract, allow_typevars) + return handle_match!(cases, match, argtypes, info, flag, state; allow_abstract, allow_typevars, override_effects) end end @@ -1444,14 +1500,14 @@ end function handle_match!(cases::Vector{InliningCase}, match::MethodMatch, argtypes::Vector{Any}, @nospecialize(info::CallInfo), flag::UInt8, state::InliningState; - allow_abstract::Bool, allow_typevars::Bool) + allow_abstract::Bool, allow_typevars::Bool, override_effects::Effects) 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 (unless unmatched type parameters are present) !allow_typevars && _any(case->case.sig === spec_types, cases) && return true - item = analyze_method!(match, argtypes, info, flag, state; allow_typevars) + item = analyze_method!(match, argtypes, info, flag, state; allow_typevars, override_effects) item === nothing && return false push!(cases, InliningCase(spec_types, item)) return true @@ -1526,8 +1582,13 @@ function handle_opaque_closure_call!(todo::Vector{Pair{Int,Any}}, mi = result.result.linfo validate_sparams(mi.sparam_vals) || return nothing item = resolve_todo(mi, result.result, sig.argtypes, info, flag, state) - elseif isa(result, ConcreteResult) && may_inline_concrete_result(result) - item = concrete_result_item(result, 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 else item = analyze_method!(info.match, sig.argtypes, info, flag, state; allow_typevars=false) end diff --git a/test/compiler/inline.jl b/test/compiler/inline.jl index eaee673455e75..ab3dd451f82d2 100644 --- a/test/compiler/inline.jl +++ b/test/compiler/inline.jl @@ -1811,3 +1811,28 @@ let src = code_typed1((NewInstruction,Any,Any,CallInfo)) do newinst, stmt, type, @test count(iscall((src,NamedTuple)), src.code) == 0 @test count(isnew, src.code) == 1 end + +# Test that inlining can still use nothrow information from concrete-eval +# even if the result itself is too big to be inlined, and nothrow is not +# known without concrete-eval +const THE_BIG_TUPLE = ntuple(identity, 1024) +function return_the_big_tuple(err::Bool) + err && error("BAD") + return THE_BIG_TUPLE +end +@noinline function return_the_big_tuple_noinline(err::Bool) + err && error("BAD") + return THE_BIG_TUPLE +end +big_tuple_test1() = return_the_big_tuple(false)[1] +big_tuple_test2() = return_the_big_tuple_noinline(false)[1] + +@test fully_eliminated(big_tuple_test2, Tuple{}) +# Currently we don't run these cleanup passes, but let's make sure that +# if we did, inlining would be able to remove this +let ir = Base.code_ircode(big_tuple_test1, Tuple{})[1][1] + ir = Core.Compiler.compact!(ir, true) + ir = Core.Compiler.cfg_simplify!(ir) + ir = Core.Compiler.compact!(ir, true) + @test length(ir.stmts) == 1 +end