Skip to content

Improve inference for ismutable with missing sparam #46693

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

Merged
merged 1 commit into from
Sep 10, 2022
Merged

Improve inference for ismutable with missing sparam #46693

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
95 changes: 62 additions & 33 deletions base/compiler/tfuncs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ function find_tfunc(@nospecialize f)
end

const DATATYPE_TYPES_FIELDINDEX = fieldindex(DataType, :types)
const DATATYPE_NAME_FIELDINDEX = fieldindex(DataType, :name)

##########
# tfuncs #
Expand Down Expand Up @@ -823,7 +824,11 @@ function getfield_nothrow(@nospecialize(s00), @nospecialize(name), boundscheck::
if isa(s, Union)
return getfield_nothrow(rewrap_unionall(s.a, s00), name, boundscheck) &&
getfield_nothrow(rewrap_unionall(s.b, s00), name, boundscheck)
elseif isa(s, DataType)
elseif isType(s)
Copy link
Member

Choose a reason for hiding this comment

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

isconstType?

Copy link
Member

Choose a reason for hiding this comment

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

(otherwise, it might be say s = UnionAll (isa DataType) here, but at runtime turn into s = Union or s = 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.

Yes, since we put in the extra checks below, that probably needs to be reflected here.

sv = s.parameters[1]
s = s0 = typeof(sv)
end
if isa(s, DataType)
# Can't say anything about abstract types
isabstracttype(s) && return false
s.name.atomicfields == C_NULL || return false # TODO: currently we're only testing for ordering === :not_atomic
Expand Down Expand Up @@ -863,15 +868,40 @@ function getfield_tfunc(s00, name, order, boundscheck)
return getfield_tfunc(s00, name)
end
getfield_tfunc(@nospecialize(s00), @nospecialize(name)) = _getfield_tfunc(s00, name, false)

function _getfield_fieldindex(@nospecialize(s), name::Const)
nv = name.val
if isa(nv, Symbol)
nv = fieldindex(s, nv, false)
end
if isa(nv, Int)
return nv
end
return nothing
end

function _getfield_tfunc_const(@nospecialize(sv), name::Const, setfield::Bool)
if isa(name, Const)
nv = _getfield_fieldindex(typeof(sv), name)
nv === nothing && return Bottom
if isa(sv, DataType) && nv == DATATYPE_TYPES_FIELDINDEX && isdefined(sv, nv)
return Const(getfield(sv, nv))
end
if isconst(typeof(sv), nv)
if isdefined(sv, nv)
return Const(getfield(sv, nv))
end
return Union{}
end
end
return nothing
end

function _getfield_tfunc(@nospecialize(s00), @nospecialize(name), setfield::Bool)
if isa(s00, Conditional)
return Bottom # Bool has no fields
elseif isa(s00, Const) || isconstType(s00)
if !isa(s00, Const)
sv = s00.parameters[1]
else
sv = s00.val
end
elseif isa(s00, Const)
sv = s00.val
if isa(name, Const)
nv = name.val
if isa(sv, Module)
Expand All @@ -881,31 +911,15 @@ function _getfield_tfunc(@nospecialize(s00), @nospecialize(name), setfield::Bool
end
return Bottom
end
if isa(nv, Symbol)
nv = fieldindex(typeof(sv), nv, false)
end
if !isa(nv, Int)
return Bottom
end
if isa(sv, DataType) && nv == DATATYPE_TYPES_FIELDINDEX && isdefined(sv, nv)
return Const(getfield(sv, nv))
end
if isconst(typeof(sv), nv)
if isdefined(sv, nv)
return Const(getfield(sv, nv))
end
return Union{}
end
r = _getfield_tfunc_const(sv, name, setfield)
r !== nothing && return r
end
s = typeof(sv)
elseif isa(s00, PartialStruct)
s = widenconst(s00)
sty = unwrap_unionall(s)::DataType
if isa(name, Const)
nv = name.val
if isa(nv, Symbol)
nv = fieldindex(sty, nv, false)
end
nv = _getfield_fieldindex(sty, name)
if isa(nv, Int) && 1 <= nv <= length(s00.fields)
return unwrapva(s00.fields[nv])
end
Expand All @@ -917,6 +931,26 @@ function _getfield_tfunc(@nospecialize(s00), @nospecialize(name), setfield::Bool
return tmerge(_getfield_tfunc(rewrap_unionall(s.a, s00), name, setfield),
_getfield_tfunc(rewrap_unionall(s.b, s00), name, setfield))
end
if isType(s)
if isconstType(s)
sv = s00.parameters[1]
if isa(name, Const)
r = _getfield_tfunc_const(sv, name, setfield)
r !== nothing && return r
end
s = typeof(sv)
else
sv = s.parameters[1]
Copy link
Member

Choose a reason for hiding this comment

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

unwrap_unionall here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is intended to catch the cases where we are guaranteed to find a DataType at runtime. @JeffBezanson and I thought the below was sufficient to catch that, but looking at it with fresh eyes, we probably also need to exclude Tuples, since they distribute over Union (so could be a Union at runtime). Are there any other cases that you can think of that we're missing?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I think that is right, since this should be a concrete DataType at runtime. Or one of the uncommon over-wrapped types at runtime (such as found in the .sig field of a method with unbound vars), but I think we currently assume it must be the former.

if isa(sv, DataType) && isa(name, Const) && (!isType(sv) && sv !== Core.TypeofBottom)
Copy link
Member

Choose a reason for hiding this comment

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

Since you already tested above with isconstType to discover that the typeof identity of sv is unknown, this seems incorrect now to assume that the typeof identity is DataType, when the previous conditional branch just returned that it might be instead a Union or UnionAll at runtime

nv = _getfield_fieldindex(DataType, name)
if nv == DATATYPE_NAME_FIELDINDEX
# N.B. This doesn't work in general, because
Copy link
Member

Choose a reason for hiding this comment

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

because?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, whoops not sure how that line got dropped. Should have said because the other fields are not guaranteed to not depend on type parameters.

return Const(sv.name)
end
s = DataType
end
end
end
isa(s, DataType) || return Any
isabstracttype(s) && return Any
if s <: Tuple && !hasintersect(widenconst(name), Int)
Expand Down Expand Up @@ -972,13 +1006,8 @@ function _getfield_tfunc(@nospecialize(s00), @nospecialize(name), setfield::Bool
end
return t
end
fld = name.val
if isa(fld, Symbol)
fld = fieldindex(s, fld, false)
end
if !isa(fld, Int)
return Bottom
end
fld = _getfield_fieldindex(s, name)
fld === nothing && return Bottom
if s <: Tuple && fld >= nf && isvarargtype(ftypes[nf])
return rewrap_unionall(unwrapva(ftypes[nf]), s00)
end
Expand Down
1 change: 1 addition & 0 deletions test/compiler/inference.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1588,6 +1588,7 @@ g23024(TT::Tuple{DataType}) = f23024(TT[1], v23024)
@test g23024((UInt8,)) === 2

@test !Core.Compiler.isconstType(Type{typeof(Union{})}) # could be Core.TypeofBottom or Type{Union{}} at runtime
@test !isa(Core.Compiler.getfield_tfunc(Type{Core.TypeofBottom}, Core.Compiler.Const(:name)), Core.Compiler.Const)
@test Base.return_types(supertype, (Type{typeof(Union{})},)) == Any[Any]

# issue #23685
Expand Down
4 changes: 4 additions & 0 deletions test/compiler/inline.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1496,3 +1496,7 @@ call_twice_sitofp(x::Int) = twice_sitofp(x, 2)
let src = code_typed1(call_twice_sitofp, (Int,))
@test count(iscall((src, Base.sitofp)), src.code) == 1
end

# Test getfield modeling of Type{Ref{_A}} where _A
@test Core.Compiler.getfield_tfunc(Type, Core.Compiler.Const(:parameters)) !== Union{}
@test fully_eliminated(Base.ismutable, Tuple{Base.RefValue})