Improve precision of tmeet for vararg PartialStruct
#39437
Conversation
| if isa(ti, DataType) && ti.name === Tuple.name | ||
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Do you think it is worthwhile to extend these for UnionAll and Union (and use them here)? Maybe postpone to a follow-up PR?
| end | ||
| end | ||
| if isva && isvarargtype(v.fields[end]) |
There was a problem hiding this comment.
!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.
base/compiler/typelimits.jl
Outdated
| end | ||
| new_fields = Vector{Any}(undef, num_fields) | ||
| for i = 1:num_fields | ||
| new_fields[i] = tmeet(getfield_tfunc(v, Const(i)), widenconst(getfield_tfunc(ti, Const(i)))) |
There was a problem hiding this comment.
t is actually expected to be a slightly stronger constraint here. We already know that the LHS is already <: widev (by original construction), so there's nothing to be gained by computing the tmeet with it. There's a (small) chance that typeintersect was approximate however, and thus wider than t.
There was a problem hiding this comment.
By similar logic, the final return operation of this function typeintersect(widenconst(v), t) can also be strengthen:
...
elseif isa(v, Type)
return typeintersect(v, t)
elseif !has_free_typevars(t) && typeintersect(widenconst(v), t) === Union{}
return Bottom
end
return v # this leaf type can't be improved by intersection with type `t`
(which also subsumes the conditionals for Const and Conditional)
There was a problem hiding this comment.
tis actually expected to be a slightly stronger constraint here.
Um, yes, I had been pondering t vs. ti for a moment here, basically came to the same conclusion, and then chose ti for reasons I can't remember. I'll change this to t (or report back in case I remember what made we pick ti instead).
There was a problem hiding this comment.
Ah, yes, I've even added a test for this: if v.typ == Tuple{Int,Vararg{Number}} and t = Union{Tuple{Int, Int, Int}, Tuple{Vararg{Float64}}}, then ti == Tuple{Int, Int, Int}. And tmeet(Number, getfield_tfunc(ti, Const(2)) == Int while tmeet(Number, getfield_tfunc(t, Const(2)) == Union{Int,Float64}. (And this doesn't depend on Vararg, that's just what the test does.)
Admittedly, a Union on the RHS of a type-assert might not be to worry about too much, so maybe still better to use t, live with the imprecision in this case but be safe against approximation in intersection? Not sure.
There was a problem hiding this comment.
Regarding the second comment (about the final return), I took that as inspiration for a slightly different restructuring of the code. Please take a look whether you like. That also made m wonder about the handling of t with free typevars here, see the respective comment.
There was a problem hiding this comment.
getfield_tfunc(v, Const(i)) seems so much more costly than v.fields[i], though perhaps it isn't significant overall. How about:
| new_fields[i] = tmeet(getfield_tfunc(v, Const(i)), widenconst(getfield_tfunc(ti, Const(i)))) | |
| new_fields[i] = tmeet(unwrapva(v.fields[min(i, end)]), widenconst(getfield_tfunc(ti, Const(i)))) |
There was a problem hiding this comment.
Included in rebase.
martinholters
force-pushed
the
mh/improve-tmeet-for-vararg-partialstruct
branch
from
9a6774c to
e1aaad6
Compare
| if isa(v, Type) | ||
| return typeintersect(v, t) | ||
| end | ||
| has_free_typevars(t) && return v |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
| if new_fields[i] === Bottom | ||
| return Bottom | ||
| end | ||
| if isa(ti, DataType) && ti.name === Tuple.name |
There was a problem hiding this comment.
Didn't we just assert that ti.name === Tuple.name on the previous line?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
martinholters
force-pushed
the
mh/improve-tmeet-for-vararg-partialstruct
branch
from
e1aaad6 to
b4434a6
Compare
Done.
Anything specific you have in mind? |
There was a problem hiding this comment.
Seems like the remaining case to consider was:
julia> Core.Compiler.tmeet(
Core.Compiler.PartialStruct(Tuple{Integer, Vararg{Float64}}, Any[Core.Compiler.Const(3), Any, Any]),
Tuple{Int, Vararg{AbstractFloat}})
We likely should not be able to form that, since tuple_tfunc expects the number of parameters to equal the number of arguments, so we should not be able to get here with a varargs PartialTuple, but may need to make sure that case really does not appear.
|
Not quite sure what you are getting at. Of course we can form varargs Just to be clear, the two minimally-modified-to-obtain-consistency versions of above example work as they should with this PR: julia> Core.Compiler.tmeet(
Core.Compiler.PartialStruct(Tuple{Integer, Vararg{Float64}}, Any[Core.Compiler.Const(3), Vararg{Any}]),
Tuple{Int, Vararg{AbstractFloat}})
Core.PartialStruct(Tuple{Int64, Vararg{Float64}}, Any[Core.Const(3), Vararg{Float64}])
julia> Core.Compiler.tmeet(
Core.Compiler.PartialStruct(Tuple{Integer, Float64, Float64}, Any[Core.Compiler.Const(3), Any, Any]),
Tuple{Int, Vararg{AbstractFloat}})
Core.PartialStruct(Tuple{Int64, Float64, Float64}, Any[Core.Const(3), Float64, Float64])(And it's unclear to me which one we should pick in case we go the handle-gracefully route.) |
Redo of #39402 against master. It was a good idea not to merge #39402 as it likely would have had problems in corner cases. I tried to be more defensive here, but wouldn't be surprised if some of my (implicit) assumptions still turn out to be wrong...