From 8b4b65263cd95bfb4f4e52984da556bb7a63cd16 Mon Sep 17 00:00:00 2001 From: Ian Atol Date: Tue, 16 Nov 2021 19:20:11 -0500 Subject: [PATCH 1/4] Relax constraints on inlining for some single calls --- base/compiler/ssair/inlining.jl | 32 ++++++++++++++------------------ test/compiler/inline.jl | 30 ++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl index efe32441b64e0..e0813b59837a3 100644 --- a/base/compiler/ssair/inlining.jl +++ b/base/compiler/ssair/inlining.jl @@ -1176,26 +1176,22 @@ function analyze_single_call!( end end - # if the signature is fully covered and there is only one applicable method, + # if the signature is fully or mostly covered and there is only one applicable method, # we can try to inline it even if the signature is not a dispatch tuple - if atype <: signature_union - if length(cases) == 0 && only_method isa Method - if length(infos) > 1 - (metharg, methsp) = ccall(:jl_type_intersection_with_env, Any, (Any, Any), - atype, only_method.sig)::SimpleVector - match = MethodMatch(metharg, methsp::SimpleVector, only_method, true) - else - meth = meth::MethodLookupResult - @assert length(meth) == 1 - match = meth[1] - end - item = analyze_method!(match, argtypes, flag, state) - item === nothing && return - push!(cases, InliningCase(match.spec_types, item)) - fully_covered = true + if length(cases) == 0 && only_method isa Method + if length(infos) > 1 + (metharg, methsp) = ccall(:jl_type_intersection_with_env, Any, (Any, Any), + atype, only_method.sig)::SimpleVector + match = MethodMatch(metharg, methsp::SimpleVector, only_method, true) + else + meth = meth::MethodLookupResult + @assert length(meth) == 1 + match = meth[1] end - else - fully_covered = false + item = analyze_method!(match, argtypes, flag, state) + item === nothing && return + push!(cases, InliningCase(match.spec_types, item)) + fully_covered = atype <: match.spec_types end # If we only have one case and that case is fully covered, we may either diff --git a/test/compiler/inline.jl b/test/compiler/inline.jl index 36acf4a8d299f..2e5cd1d621d03 100644 --- a/test/compiler/inline.jl +++ b/test/compiler/inline.jl @@ -817,3 +817,33 @@ let invoke(xs) = validate_unionsplit_inlining(true, xs[1]) @test invoke(Any[10]) === false end + +# issue 43104 + +@inline isGoodType(@nospecialize x::Type) = + x !== Any && !(@noinline Base.has_free_typevars(x)) +let # aggressive static dispatch of single, abstract method match + src = code_typed((Type, Any,)) do x, y + isGoodType(x), isGoodType(y) + end |> only |> first + # both callsite should be inlined + @test count(isinvoke(:has_free_typevars), src.code) == 2 + # `isGoodType(y::Any)` isn't fully covered, thus a runtime type check and fallback dynamic dispatch should be inserted + @test count(iscall((src,isGoodType)), src.code) == 1 +end + +@noinline function checkBadType!(@nospecialize x::Type) + if x === Any || Base.has_free_typevars(x) + println(x) + end + return nothing +end +let # aggressive inlining of single, abstract method match + src = code_typed((Type, Any,)) do x, y + checkBadType!(x), checkBadType!(y) + end |> only |> first + # both callsite should be resolved statically + @test count(isinvoke(:checkBadType!), src.code) == 2 + # `checkBadType!(y::Any)` isn't fully covered, thus a runtime type check and fallback dynamic dispatch should be inserted + @test count(iscall((src,checkBadType!)), src.code) == 1 +end From 78470e0f6902c814edf957a1e0aa525c5135f6fc Mon Sep 17 00:00:00 2001 From: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Date: Thu, 18 Nov 2021 01:34:50 +0900 Subject: [PATCH 2/4] Apply suggestions from code review --- base/compiler/ssair/inlining.jl | 3 +++ test/compiler/inline.jl | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl index e0813b59837a3..59ac9a10a32de 100644 --- a/base/compiler/ssair/inlining.jl +++ b/base/compiler/ssair/inlining.jl @@ -1192,6 +1192,9 @@ function analyze_single_call!( item === nothing && return push!(cases, InliningCase(match.spec_types, item)) fully_covered = atype <: match.spec_types + else + fully_covered &= atype <: signature_union + end end # If we only have one case and that case is fully covered, we may either diff --git a/test/compiler/inline.jl b/test/compiler/inline.jl index 2e5cd1d621d03..f6b403c2cf476 100644 --- a/test/compiler/inline.jl +++ b/test/compiler/inline.jl @@ -822,11 +822,11 @@ end @inline isGoodType(@nospecialize x::Type) = x !== Any && !(@noinline Base.has_free_typevars(x)) -let # aggressive static dispatch of single, abstract method match +let # aggressive inlining of single, abstract method match src = code_typed((Type, Any,)) do x, y isGoodType(x), isGoodType(y) end |> only |> first - # both callsite should be inlined + # both callsites should be inlined @test count(isinvoke(:has_free_typevars), src.code) == 2 # `isGoodType(y::Any)` isn't fully covered, thus a runtime type check and fallback dynamic dispatch should be inserted @test count(iscall((src,isGoodType)), src.code) == 1 @@ -838,11 +838,11 @@ end end return nothing end -let # aggressive inlining of single, abstract method match +let # aggressive static dispatch of single, abstract method match src = code_typed((Type, Any,)) do x, y checkBadType!(x), checkBadType!(y) end |> only |> first - # both callsite should be resolved statically + # both callsites should be resolved statically @test count(isinvoke(:checkBadType!), src.code) == 2 # `checkBadType!(y::Any)` isn't fully covered, thus a runtime type check and fallback dynamic dispatch should be inserted @test count(iscall((src,checkBadType!)), src.code) == 1 From 8cc91a60ab7e10d9ff210dbe72b7dd05c14ff5da Mon Sep 17 00:00:00 2001 From: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Date: Thu, 18 Nov 2021 01:35:46 +0900 Subject: [PATCH 3/4] Update base/compiler/ssair/inlining.jl --- base/compiler/ssair/inlining.jl | 1 - 1 file changed, 1 deletion(-) diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl index 59ac9a10a32de..b92a78cfcc3f7 100644 --- a/base/compiler/ssair/inlining.jl +++ b/base/compiler/ssair/inlining.jl @@ -1195,7 +1195,6 @@ function analyze_single_call!( else fully_covered &= atype <: signature_union end - end # If we only have one case and that case is fully covered, we may either # be able to do the inlining now (for constant cases), or push it directly From 4a4f363672f060577149e2c5f7f0b8e2144dcd41 Mon Sep 17 00:00:00 2001 From: Shuhei Kadowaki Date: Tue, 23 Nov 2021 10:58:19 +0900 Subject: [PATCH 4/4] inline abstract, single, constant-prop'ed match --- base/compiler/ssair/inlining.jl | 20 +++++++++----------- test/compiler/inline.jl | 13 +++++++++++++ 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl index b92a78cfcc3f7..003c93e219b33 100644 --- a/base/compiler/ssair/inlining.jl +++ b/base/compiler/ssair/inlining.jl @@ -1191,7 +1191,7 @@ function analyze_single_call!( item = analyze_method!(match, argtypes, flag, state) item === nothing && return push!(cases, InliningCase(match.spec_types, item)) - fully_covered = atype <: match.spec_types + fully_covered = match.fully_covers else fully_covered &= atype <: signature_union end @@ -1242,17 +1242,15 @@ function maybe_handle_const_call!( # if the signature is fully covered and there is only one applicable method, # we can try to inline it even if the signature is not a dispatch tuple - if atype <: signature_union - if length(cases) == 0 && length(results) == 1 - (; mi) = item = InliningTodo(results[1]::InferenceResult, argtypes) - state.mi_cache !== nothing && (item = resolve_todo(item, state, flag)) - validate_sparams(mi.sparam_vals) || return true - item === nothing && return true - push!(cases, InliningCase(mi.specTypes, item)) - fully_covered = true - end + if length(cases) == 0 && length(results) == 1 + (; mi) = item = InliningTodo(results[1]::InferenceResult, argtypes) + state.mi_cache !== nothing && (item = resolve_todo(item, state, flag)) + validate_sparams(mi.sparam_vals) || return true + item === nothing && return true + push!(cases, InliningCase(mi.specTypes, item)) + fully_covered = atype <: mi.specTypes else - fully_covered = false + fully_covered &= atype <: signature_union end # If we only have one case and that case is fully covered, we may either diff --git a/test/compiler/inline.jl b/test/compiler/inline.jl index f6b403c2cf476..53a7c9b35fb38 100644 --- a/test/compiler/inline.jl +++ b/test/compiler/inline.jl @@ -832,6 +832,19 @@ let # aggressive inlining of single, abstract method match @test count(iscall((src,isGoodType)), src.code) == 1 end +@inline isGoodType2(cnd, @nospecialize x::Type) = + x !== Any && !(@noinline (cnd ? Core.Compiler.isType : Base.has_free_typevars)(x)) +let # aggressive inlining of single, abstract method match (with constant-prop'ed) + src = code_typed((Type, Any,)) do x, y + isGoodType2(true, x), isGoodType2(true, y) + end |> only |> first + # both callsite should be inlined with constant-prop'ed result + @test count(isinvoke(:isType), src.code) == 2 + @test count(isinvoke(:has_free_typevars), src.code) == 0 + # `isGoodType(y::Any)` isn't fully convered, thus a runtime type check and fallback dynamic dispatch should be inserted + @test count(iscall((src,isGoodType2)), src.code) == 1 +end + @noinline function checkBadType!(@nospecialize x::Type) if x === Any || Base.has_free_typevars(x) println(x)