Skip to content

Commit

Permalink
inlining: make union splitting account for uncovered call (#48455)
Browse files Browse the repository at this point in the history
#44421 changed the union-splitting to skip generating
unnecessary fallback dynamic dispatch call when there is any fully
covered call.
But it turned out that this is only valid when there is any fully
covered call in matches for all signatures that inference split, and it
is invalid if there is any union split signature against which any
uncovered call is found.

Consider the following example:

    # case 1
        # def
        nosplit(::Any) = [...]
        nosplit(::Int) = [...]
        # call
        nosplit(a::Any)
            split1: a::Any ┬ nosplit(a::Int)
                           └ nosplit(a::Any) # fully covers split1

    # case 2
        # def
        convert(::Type{T}, ::T) = T
        # call
        convert(::Type{Union{Bool,Tuple{Int,String}}}, a::Union{Bool,Tuple{Int,Any}})
            split1: a::Bool           ─ convert(::Type{Bool}, ::Bool)                           # fully covers split1
            split2: a::Tuple{Int,Any} ─ convert(::Type{Tuple{Int,String}}, ::Tuple{Int,String}) # NOT fully covers split2

#44421 allows us to optimize the the first case, but
handles the second case wrongly. This commit fixes it up while still
optimizing the first case.

fix #48397.
  • Loading branch information
aviatesk authored Jan 30, 2023
1 parent 69a966c commit 7e8515c
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 7 deletions.
14 changes: 7 additions & 7 deletions base/compiler/ssair/inlining.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1335,14 +1335,13 @@ function compute_inlining_cases(@nospecialize(info::CallInfo), flag::UInt8, sig:
nunion === nothing && return nothing
cases = InliningCase[]
argtypes = sig.argtypes
local any_fully_covered = false
local handled_all_cases::Bool = true
local revisit_idx = nothing
local only_method = nothing
local meth::MethodLookupResult
local all_result_count = 0
local joint_effects::Effects = EFFECTS_TOTAL
local nothrow::Bool = true
local fully_covered::Bool = true
for i = 1:nunion
meth = getsplit(info, i)
if meth.ambig
Expand All @@ -1364,12 +1363,12 @@ function compute_inlining_cases(@nospecialize(info::CallInfo), flag::UInt8, sig:
only_method = false
end
end
local split_fully_covered::Bool = false
for (j, match) in enumerate(meth)
all_result_count += 1
result = getresult(info, all_result_count)
joint_effects = merge_effects(joint_effects, info_effects(result, match, state))
nothrow &= match.fully_covers
any_fully_covered |= match.fully_covers
split_fully_covered |= match.fully_covers
if !validate_sparams(match.sparams)
if !match.fully_covers
handled_all_cases = false
Expand All @@ -1386,9 +1385,10 @@ function compute_inlining_cases(@nospecialize(info::CallInfo), flag::UInt8, sig:
result, match, argtypes, info, flag, state; allow_abstract=true, allow_typevars=false)
end
end
fully_covered &= split_fully_covered
end

joint_effects = Effects(joint_effects; nothrow)
joint_effects = Effects(joint_effects; nothrow=fully_covered)

if handled_all_cases && revisit_idx !== nothing
# we handled everything except one match with unmatched sparams,
Expand All @@ -1415,13 +1415,13 @@ function compute_inlining_cases(@nospecialize(info::CallInfo), flag::UInt8, sig:
end
handle_any_const_result!(cases,
result, match, argtypes, info, flag, state; allow_abstract=true, allow_typevars=true)
any_fully_covered = handled_all_cases = match.fully_covers
fully_covered = handled_all_cases = match.fully_covers
elseif !handled_all_cases
# if we've not seen all candidates, union split is valid only for dispatch tuples
filter!(case::InliningCase->isdispatchtuple(case.sig), cases)
end

return cases, (handled_all_cases & any_fully_covered), joint_effects
return cases, (handled_all_cases & fully_covered), joint_effects
end

function handle_call!(todo::Vector{Pair{Int,Any}},
Expand Down
19 changes: 19 additions & 0 deletions test/compiler/inline.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1910,3 +1910,22 @@ function elim_full_ir(y)
end

@test fully_eliminated(elim_full_ir, Tuple{Int})

# union splitting should account for uncovered call signature
# https://github.com/JuliaLang/julia/issues/48397
f48397(::Bool) = :ok
f48397(::Tuple{String,String}) = :ok
let src = code_typed1((Union{Bool,Tuple{String,Any}},)) do x
f48397(x)
end
@test any(iscall((src, f48397)), src.code)
end
g48397::Union{Bool,Tuple{String,Any}} = ("48397", 48397)
@test_throws MethodError let
Base.Experimental.@force_compile
f48397(g48397)
end
@test_throws MethodError let
Base.Experimental.@force_compile
convert(Union{Bool,Tuple{String,String}}, g48397)
end

0 comments on commit 7e8515c

Please sign in to comment.