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

inlining: make union splitting account for uncovered call #48455

Merged
merged 1 commit into from
Jan 30, 2023
Merged

inlining: make union splitting account for uncovered call #48455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 7 additions & 7 deletions base/compiler/ssair/inlining.jl
View file
Open in desktop
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
View file
Open in desktop
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
Comment on lines +1924 to +1927
Copy link
Member

Choose a reason for hiding this comment

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

You can make these tests more reliable by looking at the return value here

Suggested change
@test_throws MethodError let
Base.Experimental.@force_compile
f48397(g48397)
end
let err = (@test_throws MethodError let
Base.Experimental.@force_compile
f48397(g48397)
end).value
@test err.f === f48397 && err.args === (g48397,)
end

@test_throws MethodError let
Base.Experimental.@force_compile
convert(Union{Bool,Tuple{String,String}}, g48397)
end