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

Jump to bottom

inference: fix bad effects for recursion #51092

Merged
merged 3 commits into from
Sep 1, 2023
Merged

inference: fix bad effects for recursion #51092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8307dbf
improve effects of typejoin and simplify implementation of Tuple _com…
vtjnash Aug 30, 2023
00c6ea7
inference: teach tfunc that typeof tuples must have fixed lengths
vtjnash Aug 30, 2023
15f7db8
inference: fix bad effects for recursion
vtjnash Aug 28, 2023
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
36 changes: 18 additions & 18 deletions base/boot.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,11 +249,25 @@ macro nospecialize(x)
_expr(:meta, :nospecialize, x)
end

TypeVar(n::Symbol) = _typevar(n, Union{}, Any)
TypeVar(n::Symbol, @nospecialize(ub)) = _typevar(n, Union{}, ub)
TypeVar(n::Symbol, @nospecialize(lb), @nospecialize(ub)) = _typevar(n, lb, ub)
_is_internal(__module__) = __module__ === Core
# can be used in place of `@assume_effects :foldable` (supposed to be used for bootstrapping)
macro _foldable_meta()
return _is_internal(__module__) && _expr(:meta, _expr(:purity,
#=:consistent=#true,
#=:effect_free=#true,
#=:nothrow=#false,
#=:terminates_globally=#true,
#=:terminates_locally=#false,
#=:notaskstate=#true,
#=:inaccessiblememonly=#true,
#=:noub=#true))
end

UnionAll(v::TypeVar, @nospecialize(t)) = ccall(:jl_type_unionall, Any, (Any, Any), v, t)
# n.b. the effects and model of these is refined in inference abstractinterpretation.jl
TypeVar(@nospecialize(n)) = _typevar(n::Symbol, Union{}, Any)
TypeVar(@nospecialize(n), @nospecialize(ub)) = _typevar(n::Symbol, Union{}, ub)
TypeVar(@nospecialize(n), @nospecialize(lb), @nospecialize(ub)) = _typevar(n::Symbol, lb, ub)
UnionAll(@nospecialize(v), @nospecialize(t)) = ccall(:jl_type_unionall, Any, (Any, Any), v::TypeVar, t)

# simple convert for use by constructors of types in Core
# note that there is no actual conversion defined here,
Expand Down Expand Up @@ -453,20 +467,6 @@ function _Task(@nospecialize(f), reserved_stack::Int, completion_future)
return ccall(:jl_new_task, Ref{Task}, (Any, Any, Int), f, completion_future, reserved_stack)
end

_is_internal(__module__) = __module__ === Core
# can be used in place of `@assume_effects :foldable` (supposed to be used for bootstrapping)
macro _foldable_meta()
return _is_internal(__module__) && Expr(:meta, Expr(:purity,
#=:consistent=#true,
#=:effect_free=#true,
#=:nothrow=#false,
#=:terminates_globally=#true,
#=:terminates_locally=#false,
#=:notaskstate=#false,
#=:inaccessiblememonly=#false,
#=:noub=#true))
end

const NTuple{N,T} = Tuple{Vararg{T,N}}

## primitive Array constructors
Expand Down
29 changes: 19 additions & 10 deletions base/compiler/abstractinterpretation.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,7 @@ function concrete_eval_eligible(interp::AbstractInterpreter,
end
# disable concrete-evaluation if this function call is tainted by some overlayed
# method since currently there is no easy way to execute overlayed methods
add_remark!(interp, sv, "[constprop] Concrete evel disabled for overlayed methods")
add_remark!(interp, sv, "[constprop] Concrete eval disabled for overlayed methods")
end
if !any_conditional(arginfo)
return :semi_concrete_eval
Expand Down Expand Up @@ -1852,7 +1852,7 @@ function abstract_call_builtin(interp::AbstractInterpreter, f::Builtin, (; fargs
return rt
end

function abstract_call_unionall(interp::AbstractInterpreter, argtypes::Vector{Any})
function abstract_call_unionall(interp::AbstractInterpreter, argtypes::Vector{Any}, call::CallMeta)
if length(argtypes) == 3
canconst = true
a2 = argtypes[2]
Expand All @@ -1865,10 +1865,10 @@ function abstract_call_unionall(interp::AbstractInterpreter, argtypes::Vector{An
body = a3.parameters[1]
canconst = false
else
return CallMeta(Any, Effects(EFFECTS_TOTAL; nothrow), NoCallInfo())
return CallMeta(Any, Effects(EFFECTS_TOTAL; nothrow), call.info)
end
if !(isa(body, Type) || isa(body, TypeVar))
return CallMeta(Any, EFFECTS_THROWS, NoCallInfo())
return CallMeta(Any, EFFECTS_THROWS, call.info)
end
if has_free_typevars(body)
if isa(a2, Const)
Expand All @@ -1877,13 +1877,13 @@ function abstract_call_unionall(interp::AbstractInterpreter, argtypes::Vector{An
tv = a2.tv
canconst = false
else
return CallMeta(Any, EFFECTS_THROWS, NoCallInfo())
return CallMeta(Any, EFFECTS_THROWS, call.info)
end
isa(tv, TypeVar) || return CallMeta(Any, EFFECTS_THROWS, NoCallInfo())
isa(tv, TypeVar) || return CallMeta(Any, EFFECTS_THROWS, call.info)
body = UnionAll(tv, body)
end
ret = canconst ? Const(body) : Type{body}
return CallMeta(ret, Effects(EFFECTS_TOTAL; nothrow), NoCallInfo())
return CallMeta(ret, Effects(EFFECTS_TOTAL; nothrow), call.info)
end
return CallMeta(Bottom, EFFECTS_THROWS, NoCallInfo())
end
Expand Down Expand Up @@ -1998,12 +1998,20 @@ function abstract_call_known(interp::AbstractInterpreter, @nospecialize(f),
elseif la == 3
ub_var = argtypes[3]
end
# make sure generic code is prepared for inlining if needed later
call = let T = Any[Type{TypeVar}, Any, Any, Any]
resize!(T, la)
atype = Tuple{T...}
T[1] = Const(TypeVar)
abstract_call_gf_by_type(interp, f, ArgInfo(nothing, T), si, atype, sv, max_methods)
end
pT = typevar_tfunc(𝕃ᵢ, n, lb_var, ub_var)
effects = builtin_effects(𝕃ᵢ, Core._typevar, ArgInfo(nothing,
Any[Const(Core._typevar), n, lb_var, ub_var]), pT)
return CallMeta(pT, effects, NoCallInfo())
return CallMeta(pT, effects, call.info)
elseif f === UnionAll
return abstract_call_unionall(interp, argtypes)
call = abstract_call_gf_by_type(interp, f, ArgInfo(nothing, Any[Const(UnionAll), Any, Any]), si, Tuple{Type{UnionAll}, Any, Any}, sv, max_methods)
return abstract_call_unionall(interp, argtypes, call)
elseif f === Tuple && la == 2
aty = argtypes[2]
ty = isvarargtype(aty) ? unwrapva(aty) : widenconst(aty)
Expand All @@ -2021,13 +2029,14 @@ function abstract_call_known(interp::AbstractInterpreter, @nospecialize(f),
end
elseif la == 3 && istopfunction(f, :!==)
# mark !== as exactly a negated call to ===
call = abstract_call_gf_by_type(interp, f, ArgInfo(fargs, Any[Const(f), Any, Any]), si, Tuple{typeof(f), Any, Any}, sv, max_methods)
rty = abstract_call_known(interp, (===), arginfo, si, sv, max_methods).rt
if isa(rty, Conditional)
return CallMeta(Conditional(rty.slot, rty.elsetype, rty.thentype), EFFECTS_TOTAL, NoCallInfo()) # swap if-else
elseif isa(rty, Const)
return CallMeta(Const(rty.val === false), EFFECTS_TOTAL, MethodResultPure())
end
return CallMeta(rty, EFFECTS_TOTAL, NoCallInfo())
return call
elseif la == 3 && istopfunction(f, :(>:))
# mark issupertype as a exact alias for issubtype
# swap T1 and T2 arguments and call <:
Expand Down
8 changes: 6 additions & 2 deletions base/compiler/tfuncs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,8 +731,12 @@ function typeof_concrete_vararg(t::DataType)
for i = 1:np
p = t.parameters[i]
if i == np && isvarargtype(p)
if isdefined(p, :T) && !isdefined(p, :N) && isconcretetype(p.T)
return Type{Tuple{t.parameters[1:np-1]..., Vararg{p.T, N}}} where N
if isdefined(p, :T) && isconcretetype(p.T)
t = Type{Tuple{t.parameters[1:np-1]..., Vararg{p.T, N}}} where N
if isdefined(p, :N)
return t{p.N}
end
return t
end
elseif !isconcretetype(p)
break
Expand Down
59 changes: 34 additions & 25 deletions base/compiler/typeinfer.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,34 @@ function cycle_fix_limited(@nospecialize(typ), sv::InferenceState)
return typ
end

function adjust_effects(ipo_effects::Effects, def::Method)
# override the analyzed effects using manually annotated effect settings
override = decode_effects_override(def.purity)
if is_effect_overridden(override, :consistent)
ipo_effects = Effects(ipo_effects; consistent=ALWAYS_TRUE)
end
if is_effect_overridden(override, :effect_free)
ipo_effects = Effects(ipo_effects; effect_free=ALWAYS_TRUE)
end
if is_effect_overridden(override, :nothrow)
ipo_effects = Effects(ipo_effects; nothrow=true)
end
if is_effect_overridden(override, :terminates_globally)
ipo_effects = Effects(ipo_effects; terminates=true)
end
if is_effect_overridden(override, :notaskstate)
ipo_effects = Effects(ipo_effects; notaskstate=true)
end
if is_effect_overridden(override, :inaccessiblememonly)
ipo_effects = Effects(ipo_effects; inaccessiblememonly=ALWAYS_TRUE)
end
if is_effect_overridden(override, :noub)
ipo_effects = Effects(ipo_effects; noub=ALWAYS_TRUE)
end
return ipo_effects
end


function adjust_effects(sv::InferenceState)
ipo_effects = sv.ipo_effects

Expand Down Expand Up @@ -478,28 +506,7 @@ function adjust_effects(sv::InferenceState)
# override the analyzed effects using manually annotated effect settings
def = sv.linfo.def
if isa(def, Method)
override = decode_effects_override(def.purity)
if is_effect_overridden(override, :consistent)
ipo_effects = Effects(ipo_effects; consistent=ALWAYS_TRUE)
end
if is_effect_overridden(override, :effect_free)
ipo_effects = Effects(ipo_effects; effect_free=ALWAYS_TRUE)
end
if is_effect_overridden(override, :nothrow)
ipo_effects = Effects(ipo_effects; nothrow=true)
end
if is_effect_overridden(override, :terminates_globally)
ipo_effects = Effects(ipo_effects; terminates=true)
end
if is_effect_overridden(override, :notaskstate)
ipo_effects = Effects(ipo_effects; notaskstate=true)
end
if is_effect_overridden(override, :inaccessiblememonly)
ipo_effects = Effects(ipo_effects; inaccessiblememonly=ALWAYS_TRUE)
end
if is_effect_overridden(override, :noub)
ipo_effects = Effects(ipo_effects; noub=ALWAYS_TRUE)
end
ipo_effects = adjust_effects(ipo_effects, def)
end

return ipo_effects
Expand Down Expand Up @@ -850,16 +857,18 @@ function typeinf_edge(interp::AbstractInterpreter, method::Method, @nospecialize
end
typeinf(interp, frame)
update_valid_age!(caller, frame.valid_worlds)
edge = is_inferred(frame) ? mi : nothing
return EdgeCallResult(frame.bestguess, edge, frame.ipo_effects) # effects are adjusted already within `finish`
isinferred = is_inferred(frame)
edge = isinferred ? mi : nothing
effects = isinferred ? frame.ipo_effects : adjust_effects(Effects(), method) # effects are adjusted already within `finish` for ipo_effects
vtjnash marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I came up with this fix for typejoin accuracy for external AbstractInterpreter with overlay table:

Suggested change
effects = isinferred ? frame.ipo_effects : adjust_effects(Effects(), method) # effects are adjusted already within `finish` for ipo_effects
if isinferred
effects = frame.ipo_effects # effects are adjusted already within `finish`
else
# if this method isn't overlayed, we don't need to taint `:nonoverlayed`
effects = is_nonoverlayed(method) ? EFFECTS_UNKNOWN : Effects()
effects = adjust_effects(effects, method)
end

But I just realized this isn't correct, because we can't guarantee that any overlay methods aren't involved within a recursive call graph..
And now I think this may be more problematic that I thought, because now we basically make external AbstractInterpreter with overlay table unable to perform concrete eval on any recursive methods.

I think there are two options:

  1. make inference keep track of a set of methods that appear within a call graph and check if there are any overlay methods here, which sounds a bit complicated but at the same time may be useful for the other analyses too
  2. move forward effects: allow override of :nonoverlayed effect bit (and rename it to :native_executable) #51080 and annotate :nonoverlayed typejoin, which seems awkward honestly, because it is obvious that typejoin is not overlayed

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

typejoin is probably considered to be :total, which would possibly already work. The only reason this method is a problem is that it is annotated :terminates, since otherwise inference couldn't derive that information itself anyways.

return EdgeCallResult(frame.bestguess, edge, effects)
elseif frame === true
# unresolvable cycle
return EdgeCallResult(Any, nothing, Effects())
end
# return the current knowledge about this cycle
frame = frame::InferenceState
update_valid_age!(caller, frame.valid_worlds)
return EdgeCallResult(frame.bestguess, nothing, adjust_effects(frame))
return EdgeCallResult(frame.bestguess, nothing, adjust_effects(Effects(), method))
end

function cached_return_type(code::CodeInstance)
Expand Down
5 changes: 4 additions & 1 deletion base/essentials.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ macro _foldable_meta()
#=:nothrow=#false,
#=:terminates_globally=#true,
#=:terminates_locally=#false,
#=:notaskstate=#false,
#=:notaskstate=#true,
#=:inaccessiblememonly=#true,
#=:noub=#true))
end
Expand Down Expand Up @@ -264,6 +264,9 @@ end
macro _propagate_inbounds_meta()
return Expr(:meta, :inline, :propagate_inbounds)
end
macro _nospecializeinfer_meta()
return Expr(:meta, :nospecializeinfer)
end

function iterate end

Expand Down
13 changes: 8 additions & 5 deletions base/promotion.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@ Number
```
"""
typejoin() = Bottom
typejoin(@nospecialize(t)) = t
typejoin(@nospecialize(t), ts...) = (@_foldable_meta; typejoin(t, typejoin(ts...)))
typejoin(@nospecialize(t)) = (@_nospecializeinfer_meta; t)
typejoin(@nospecialize(t), ts...) = (@_foldable_meta; @_nospecializeinfer_meta; typejoin(t, typejoin(ts...)))
function typejoin(@nospecialize(a), @nospecialize(b))
@_foldable_meta
@_nospecializeinfer_meta
if isa(a, TypeVar)
return typejoin(a.ub, b)
elseif isa(b, TypeVar)
Expand Down Expand Up @@ -90,9 +91,9 @@ function typejoin(@nospecialize(a), @nospecialize(b))
elseif b <: Tuple
return Any
end
while b !== Any
while !(b === Any)
if a <: b.name.wrapper
while a.name !== b.name
while !(a.name === b.name)
a = supertype(a)::DataType
end
if a.name === Type.body.name
Expand Down Expand Up @@ -139,6 +140,7 @@ end
# (Core.Compiler.isnotbrokensubtype), use only simple types for `b`
function typesplit(@nospecialize(a), @nospecialize(b))
@_foldable_meta
@_nospecializeinfer_meta
if a <: b
return Bottom
end
Expand Down Expand Up @@ -239,7 +241,8 @@ function full_va_len(p::Core.SimpleVector)
end

# reduce typejoin over A[i:end]
function tailjoin(A, i)
function tailjoin(A::SimpleVector, i::Int)
@_foldable_meta
if i > length(A)
return unwrapva(A[end])
end
Expand Down
2 changes: 1 addition & 1 deletion base/reflection.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ end

iskindtype(@nospecialize t) = (t === DataType || t === UnionAll || t === Union || t === typeof(Bottom))
isconcretedispatch(@nospecialize t) = isconcretetype(t) && !iskindtype(t)
has_free_typevars(@nospecialize(t)) = ccall(:jl_has_free_typevars, Cint, (Any,), t) != 0
has_free_typevars(@nospecialize(t)) = (@_total_meta; ccall(:jl_has_free_typevars, Cint, (Any,), t) != 0)

# equivalent to isa(v, Type) && isdispatchtuple(Tuple{v}) || v === Union{}
# and is thus perhaps most similar to the old (pre-1.0) `isleaftype` query
Expand Down
44 changes: 17 additions & 27 deletions base/tuple.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,41 +199,31 @@ first(t::Tuple) = t[1]
# eltype

eltype(::Type{Tuple{}}) = Bottom
function eltype(t::Type{<:Tuple{Vararg{E}}}) where {E}
if @isdefined(E)
return E
else
# TODO: need to guard against E being miscomputed by subtyping (ref #23017)
# and compute the result manually in this case
return _compute_eltype(t)
end
end
# the <: here makes the runtime a bit more complicated (needing to check isdefined), but really helps inference
eltype(t::Type{<:Tuple{Vararg{E}}}) where {E} = @isdefined(E) ? (E isa Type ? E : Union{}) : _compute_eltype(t)
eltype(t::Type{<:Tuple}) = _compute_eltype(t)
function _tuple_unique_fieldtypes(@nospecialize t)
function _compute_eltype(@nospecialize t)
@_total_meta
types = IdSet()
has_free_typevars(t) && return Any
t´ = unwrap_unionall(t)
# Given t = Tuple{Vararg{S}} where S<:Real, the various
# unwrapping/wrapping/va-handling here will return Real
if t´ isa Union
union!(types, _tuple_unique_fieldtypes(rewrap_unionall(t´.a, t)))
union!(types, _tuple_unique_fieldtypes(rewrap_unionall(t´.b, t)))
else
for ti in (t´::DataType).parameters
push!(types, rewrap_unionall(unwrapva(ti), t))
end
return promote_typejoin(_compute_eltype(rewrap_unionall(t´.a, t)),
_compute_eltype(rewrap_unionall(t´.b, t)))
end
return Core.svec(types...)
end
function _compute_eltype(@nospecialize t)
@_total_meta # TODO: the compiler shouldn't need this
types = _tuple_unique_fieldtypes(t)
return afoldl(types...) do a, b
# if we've already reached Any, it can't widen any more
a === Any && return Any
b === Any && return Any
return promote_typejoin(a, b)
p = (t´::DataType).parameters
length(p) == 0 && return Union{}
elt = rewrap_unionall(unwrapva(p[1]), t)
elt isa Type || return Union{} # Tuple{2} is legal as a Type, but the eltype is Union{} since it is uninhabited
r = elt
for i in 2:length(p)
r === Any && return r # if we've already reached Any, it can't widen any more
elt = rewrap_unionall(unwrapva(p[i]), t)
elt isa Type || return Union{} # Tuple{2} is legal as a Type, but the eltype is Union{} since it is uninhabited
r = promote_typejoin(elt, r)
end
return r
end

# We'd like to be able to infer eltype(::Tuple), which needs to be able to
Expand Down
2 changes: 1 addition & 1 deletion test/compiler/AbstractInterpreter.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ function CC.concrete_eval_eligible(interp::Issue48097Interp,
end
@overlay Issue48097MT @noinline Core.throw_inexacterror(f::Symbol, ::Type{T}, val) where {T} = return
issue48097(; kwargs...) = return 42
@test fully_eliminated(; interp=Issue48097Interp(), retval=42) do
@test_broken fully_eliminated(; interp=Issue48097Interp(), retval=42) do
issue48097(; a=1f0, b=1.0)
end

Expand Down
Loading