Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
inference: remove union-split limit for linear signatures (#37378)
This size limit should be already be imposed elsewhere (tmerge), and
should not actually add cost to perform the union/tuple-switching when
there is no cartesian product to consider. This permits users to
explicitly demand larger union products (for example, with type-asserts
or field types) and still expect to get reliable union-splitting at any
size in single-dispatch sites.
  • Loading branch information
vtjnash committed Sep 22, 2020
1 parent b0cfb43 commit 2a36f83
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 15 deletions.
6 changes: 3 additions & 3 deletions base/compiler/abstractinterpretation.jl
Expand Up @@ -24,7 +24,7 @@ function abstract_call_gf_by_type(interp::AbstractInterpreter, @nospecialize(f),
end
valid_worlds = WorldRange()
atype_params = unwrap_unionall(atype).parameters
splitunions = 1 < countunionsplit(atype_params) <= InferenceParams(interp).MAX_UNION_SPLITTING
splitunions = 1 < unionsplitcost(atype_params) <= InferenceParams(interp).MAX_UNION_SPLITTING
mts = Core.MethodTable[]
fullmatch = Bool[]
if splitunions
Expand Down Expand Up @@ -113,7 +113,7 @@ function abstract_call_gf_by_type(interp::AbstractInterpreter, @nospecialize(f),
sigtuple = unwrap_unionall(sig)::DataType
splitunions = false
this_rt = Bottom
# TODO: splitunions = 1 < countunionsplit(sigtuple.parameters) * napplicable <= InferenceParams(interp).MAX_UNION_SPLITTING
# TODO: splitunions = 1 < unionsplitcost(sigtuple.parameters) * napplicable <= InferenceParams(interp).MAX_UNION_SPLITTING
# currently this triggers a bug in inference recursion detection
if splitunions
splitsigs = switchtupleunion(sig)
Expand Down Expand Up @@ -654,7 +654,7 @@ function abstract_apply(interp::AbstractInterpreter, @nospecialize(itft), @nospe
end
res = Union{}
nargs = length(aargtypes)
splitunions = 1 < countunionsplit(aargtypes) <= InferenceParams(interp).MAX_APPLY_UNION_ENUM
splitunions = 1 < unionsplitcost(aargtypes) <= InferenceParams(interp).MAX_APPLY_UNION_ENUM
ctypes = Any[Any[aft]]
infos = [Union{Nothing, AbstractIterationInfo}[]]
for i = 1:nargs
Expand Down
2 changes: 1 addition & 1 deletion base/compiler/ssair/inlining.jl
Expand Up @@ -1173,7 +1173,7 @@ function assemble_inline_todo!(ir::IRCode, state::InliningState)
continue
end

nu = countunionsplit(sig.atypes)
nu = unionsplitcost(sig.atypes)
if nu == 1 || nu > state.params.MAX_UNION_SPLITTING
if !isa(info, MethodMatchInfo)
if state.method_table === nothing
Expand Down
17 changes: 13 additions & 4 deletions base/compiler/typeutils.jl
Expand Up @@ -103,14 +103,23 @@ function tuple_tail_elem(@nospecialize(init), ct::Vector{Any})
return Vararg{widenconst(t)}
end

function countunionsplit(atypes::Union{SimpleVector,Vector{Any}})
# Gives a cost function over the effort to switch a tuple-union representation
# as a cartesian product, relative to the size of the original representation.
# Thus, we count the longest element as being roughly invariant to being inside
# or outside of the Tuple/Union nesting, though somewhat more expensive to be
# outside than inside because the representation is larger (because and it
# informs the callee whether any splitting is possible).
function unionsplitcost(atypes::Union{SimpleVector,Vector{Any}})
nu = 1
max = 2
for ti in atypes
if isa(ti, Union)
nu, ovf = Core.Intrinsics.checked_smul_int(nu, unionlen(ti::Union))
if ovf
return typemax(Int)
nti = unionlen(ti)
if nti > max
max, nti = nti, max
end
nu, ovf = Core.Intrinsics.checked_smul_int(nu, nti)
ovf && return typemax(Int)
end
end
return nu
Expand Down
21 changes: 14 additions & 7 deletions test/compiler/inference.jl
Expand Up @@ -769,20 +769,23 @@ g11015(::Type{Bool}, ::Bool) = 2.0

# better inference of apply (#20343)
f20343(::String, ::Int) = 1
f20343(::Int, ::String, ::Int, ::Int) = 1
f20343(::Int, ::Int, ::String, ::Int, ::Int, ::Int) = 1
f20343(::Union{Int,String}...) = Int8(1)
f20343(::Int, ::String, ::Int, ::Int) = 2
f20343(::Int, ::Int, ::String, ::Int, ::Int, ::Int) = 3
f20343(::Int, ::Int, ::Int, ::String, ::Int, ::Int, ::Int, ::Int, ::Int, ::Int, ::Int, ::Int) = 4
f20343(::Union{Int,String}...) = Int8(5)
f20343(::Any...) = "no"
function g20343()
n = rand(1:3)
i = ntuple(i->n==i ? "" : 0, 2n)::Union{Tuple{String,Int},Tuple{Int,String,Int,Int},Tuple{Int,Int,String,Int,Int,Int}}
T = Union{Tuple{String, Int}, Tuple{Int, String, Int, Int}, Tuple{Int, Int, String, Int, Int, Int}}
i = ntuple(i -> n == i ? "" : 0, 2n)::T
f20343(i...)
end
@test Base.return_types(g20343, ()) == [Int]
function h20343()
n = rand(1:3)
i = ntuple(i->n==i ? "" : 0, 3)::Union{Tuple{String,Int,Int},Tuple{Int,String,Int},Tuple{Int,Int,String}}
f20343(i..., i...)
T = Union{Tuple{String, Int, Int}, Tuple{Int, String, Int}, Tuple{Int, Int, String}}
i = ntuple(i -> n == i ? "" : 0, 3)::T
f20343(i..., i..., i..., i...)
end
@test Base.return_types(h20343, ()) == [Union{Int8, Int}]
function i20343()
Expand Down Expand Up @@ -2099,7 +2102,11 @@ end
# issue #28356
# unit test to make sure countunionsplit overflows gracefully
# we don't care what number is returned as long as it's large
@test Core.Compiler.countunionsplit(Any[Union{Int32,Int64} for i=1:80]) > 100000
@test Core.Compiler.unionsplitcost(Any[Union{Int32, Int64} for i=1:80]) > 100000
@test Core.Compiler.unionsplitcost(Any[Union{Int8, Int16, Int32, Int64}]) == 2
@test Core.Compiler.unionsplitcost(Any[Union{Int8, Int16, Int32, Int64}, Union{Int8, Int16, Int32, Int64}, Int8]) == 8
@test Core.Compiler.unionsplitcost(Any[Union{Int8, Int16, Int32, Int64}, Union{Int8, Int16, Int32}, Int8]) == 6
@test Core.Compiler.unionsplitcost(Any[Union{Int8, Int16, Int32}, Union{Int8, Int16, Int32, Int64}, Int8]) == 6

# make sure compiler doesn't hang in union splitting

Expand Down

2 comments on commit 2a36f83

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Please sign in to comment.