Skip to content

Commit

Permalink
inference: fix bad effects for recursion (#51092)
Browse files Browse the repository at this point in the history 
Effects are not converged, so they need to always be correct, and not a
bestguess, even during recursion. This could be refined, but we don't
really need to, and it might be unnecessarily costly to do so.
  • Loading branch information
@vtjnash
vtjnash authored Sep 1, 2023
2 parents befe6f8 + 15f7db8 commit 631b5c3
Show file tree
Hide file tree
Showing 11 changed files with 132 additions and 95 deletions.
36 changes: 18 additions & 18 deletions base/boot.jl
View file
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 @@ -452,20 +466,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
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
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
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
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
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
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
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
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
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

2 comments on commit 631b5c3

@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 package evaluation, I will reply here when finished:

@nanosoldier runtests(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.

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Please sign in to comment.