Skip to content

Improve precision of tmeet for vararg PartialStruct #39437

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Improve precision of tmeet for vararg PartialStruct #39437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a823309
Improve precision of `tmeet` for vararg `PartialStruct`
martinholters Jan 27, 2021
b4434a6
Restructure `tmeet` based on feedback
martinholters Feb 1, 2021
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
59 changes: 30 additions & 29 deletions base/compiler/typelimits.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,39 +586,40 @@ end
# compute typeintersect over the extended inference lattice
# where v is in the extended lattice, and t is a Type
function tmeet(@nospecialize(v), @nospecialize(t))
if isa(v, Const)
if !has_free_typevars(t) && !isa(v.val, t)
return Bottom
end
if isa(v, Type)
return typeintersect(v, t)
end
has_free_typevars(t) && return v
Copy link
Member Author

@martinholters martinholters Feb 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is basically an NFC, but it looks a bit funny this way: Is it really safe to rely on the type intersection if isa(v, Type) even if has_free_typevars(t)? And if so, why is it not safe if v is something else (then using widev = widenconst(v) of course)?

Copy link
Member

Choose a reason for hiding this comment

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

We need to stop them from appearing, but that's a project for later. Mostly likely it would actually over-estimate the intersection (which is generally what we do here too), so we only try to avoid them most of the time.

widev = widenconst(v)
if widev <: t
return v
elseif isa(v, PartialStruct)
has_free_typevars(t) && return v
widev = widenconst(v)
if widev <: t
return v
end
ti = typeintersect(widev, t)
if ti === Bottom
return Bottom
end
end
ti = typeintersect(widev, t)
if ti === Bottom
return Bottom
end
if isa(v, PartialStruct)
@assert widev <: Tuple
new_fields = Vector{Any}(undef, length(v.fields))
for i = 1:length(new_fields)
if isa(v.fields[i], Core.TypeofVararg)
new_fields[i] = v.fields[i]
else
new_fields[i] = tmeet(v.fields[i], widenconst(getfield_tfunc(t, Const(i))))
if new_fields[i] === Bottom
return Bottom
end
if isa(ti, DataType) && ti.name === Tuple.name
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we just assert that ti.name === Tuple.name on the previous line?

Copy link
Member Author

Choose a reason for hiding this comment

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

In a somewhat indirect way and with some assumptions on the well-behavedness of typeinstersect. So file this under defensive programming(and the === should be cheap enough that it's ok).

Copy link
Member Author

Choose a reason for hiding this comment

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

But, huh, is the assertion actually valid? I think it implies that if the PartialStruct is for something other than a tuple, its typ (aka widev) is concrete, so that either widev < t or typeintersect(widev, t) == Union{}. While that seems to hold at the moment, it also seems to be something that could be reasonably relaxed in the future. Maybe better replace the assertion with an if and just return the intersection result in the else branch?

num_fields = length(ti.parameters)
isva = isvatuple(ti)
else
nfields_ti = nfields_tfunc(ti)
isva = !isa(nfields_ti, Const)
num_fields = isva ? length(v.fields) : (nfields_ti::Const).val
end
Comment on lines +603 to +610
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not too happy about this. What I really want to know is: What is the minimum number of fields of ti and can there be more (vararg)? I get that for the simple case (if branch), but not quite for the else branch. This is likely not the only place we need this, so I probably just missed an elegant solution to this?

Copy link
Member

Choose a reason for hiding this comment

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

IIUC, this means that if we started with Tuple{Int, Vararg{Float64}}, and ended up with something shorter somehow, that we'd compute Tuple{Vararg{Int}} below, instead of Tuple{Vararg{Int, Float64}}. So perhaps we should write this with max(length(v.fields), (nfields_ti::Const).val::Int)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me illustrate with an example: Let

v == PartialStruct(Tuple{Int64, Vararg{Number}}, Any[Core.Const(1), Vararg{Number}]

and

t == Union{Tuple{Int,Int,Int},Tuple{Int,Float64,Float64,Float64}}

then we get ti == t. And now I'd like to figure out that ti has at least 3 fields. At present tmeet (with this PR) gives

Core.PartialStruct(Tuple{Int64, Vararg{Union{Float64, Int64}}}, Any[Core.Const(1), Vararg{Union{Float64, Int64}}])

but it could even be

Core.PartialStruct(Tuple{Int64, Union{Float64, Int64}, Union{Float64, Int64}, Vararg{Float64}}, Any[Core.Const(1), Union{Float64, Int64}, Union{Float64, Int64}, Vararg{Union{Float64, Int64}}])

(But maybe that's a bit too greedy and we leave it as is.)

Copy link
Member

Choose a reason for hiding this comment

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

datatype_min_ninitialized now gives you the "minimum number of fields" query, and isknownlength gives you the isva query (might want to add UnionAll and Union handling though, since currently they only accept DataType)

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it is worthwhile to extend these for UnionAll and Union (and use them here)? Maybe postpone to a follow-up PR?

new_fields = Vector{Any}(undef, num_fields)
for i = 1:num_fields
new_fields[i] = tmeet(unwrapva(v.fields[min(i, end)]), widenconst(getfield_tfunc(ti, Const(i))))
if new_fields[i] === Bottom
return Bottom
end
end
return tuple_tfunc(new_fields)
elseif isa(v, Conditional)
if !(Bool <: t)
return Bottom
if isva && isvarargtype(v.fields[end])
Copy link
Member Author

Choose a reason for hiding this comment

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

!isvarargtype(v.fields[end]) should imply !isva, but I wanted to be sure we don't unnecessarily form a vararg in case type intersection produces too wide a result.

new_fields[end] = Vararg{new_fields[end]}
end
return v
return tuple_tfunc(new_fields)
end
return typeintersect(widenconst(v), t)
# v is a Const or Conditional and its type is compatible with t
return v
end
12 changes: 12 additions & 0 deletions test/compiler/inference.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3146,6 +3146,18 @@ function splat_lotta_unions()
end
@test Core.Compiler.return_type(splat_lotta_unions, Tuple{}) >: Tuple{Int,Int,Int}

# issue #38971 and related precision issues
f28971() = (1, [2,3]...)::Tuple{Int,Int,Int}
@test @inferred(f28971()) == (1, 2, 3)
g28971(::Type{T}) where {T} = (1, Number[2,3]...)::T
@test Base.return_types(g28971, Tuple{Type{Tuple{Vararg{Int}}}})[1] == Tuple{Int,Vararg{Int}}
@test Base.return_types(g28971, Tuple{Type{Tuple{Int,Vararg{Int}}}})[1] == Tuple{Int,Vararg{Int}}
@test Base.return_types(g28971, Tuple{Type{Tuple{Int,Int,Vararg{Int}}}})[1] == Tuple{Int,Int,Vararg{Int}}
@test Base.return_types(g28971, Tuple{Type{Union{Tuple{Int,Int,Int},Tuple{Vararg{Float64}}}}})[1] == Tuple{Int,Int,Int}
let T = Union{Tuple{Int,Int,Int},Tuple{Int,Vararg{Float64}}}
@test T <: Base.return_types(g28971, Tuple{Type{T}})[1] <: Tuple{Int,Vararg{Union{Int,Float64}}}
end

# Bare Core.Argument in IR
@eval f_bare_argument(x) = $(Core.Argument(2))
@test Base.return_types(f_bare_argument, (Int,))[1] == Int
Expand Down