Skip to content

Commit

Permalink
add missing invoke edge for nospecialize targets (#51036)
Browse files Browse the repository at this point in the history
We need 2 edges: one for the lookup (which uses the call signature) and
one for the invoke (which uses the invoke signature). It is hard to make
a small example for this, but the test case demonstrated this issue,
particularly if inspected by `SnoopCompile.@snoopr`.

Additionally, we can do some easy optimizations on the invoke
invalidation, since in most cases we know from subtyping transativity
that it is only invalid if the method callee target is actually deleted,
and otherwise it cannot ever be partially replaced.

Fixes: #50091
Likely introduced by #49404, so marking for v1.10 backport only

(cherry picked from commit 6097140)
  • Loading branch information
vtjnash authored and KristofferC committed Sep 15, 2023
1 parent aa11508 commit 210fb22
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 22 deletions.
5 changes: 3 additions & 2 deletions base/compiler/ssair/inlining.jl
Original file line number Diff line number Diff line change
Expand Up @@ -817,8 +817,8 @@ end
function compileable_specialization(mi::MethodInstance, effects::Effects,
et::InliningEdgeTracker, @nospecialize(info::CallInfo); compilesig_invokes::Bool=true)
mi_invoke = mi
method, atype, sparams = mi.def::Method, mi.specTypes, mi.sparam_vals
if compilesig_invokes
method, atype, sparams = mi.def::Method, mi.specTypes, mi.sparam_vals
new_atype = get_compileable_sig(method, atype, sparams)
new_atype === nothing && return nothing
if atype !== new_atype
Expand All @@ -836,7 +836,8 @@ function compileable_specialization(mi::MethodInstance, effects::Effects,
return nothing
end
end
add_inlining_backedge!(et, mi)
add_inlining_backedge!(et, mi) # to the dispatch lookup
push!(et.edges, method.sig, mi_invoke) # add_inlining_backedge to the invoke call
return InvokeCase(mi_invoke, effects, info)
end

Expand Down
5 changes: 4 additions & 1 deletion src/gf.c
Original file line number Diff line number Diff line change
Expand Up @@ -2130,7 +2130,10 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
int replaced_edge;
if (invokeTypes) {
// n.b. normally we must have mi.specTypes <: invokeTypes <: m.sig (though it might not strictly hold), so we only need to check the other subtypes
replaced_edge = jl_subtype(invokeTypes, type) && is_replacing(ambig, type, m, d, n, invokeTypes, NULL, morespec);
if (jl_egal(invokeTypes, caller->def.method->sig))
replaced_edge = 0; // if invokeTypes == m.sig, then the only way to change this invoke is to replace the method itself
else
replaced_edge = jl_subtype(invokeTypes, type) && is_replacing(ambig, type, m, d, n, invokeTypes, NULL, morespec);
}
else {
replaced_edge = replaced_dispatch;
Expand Down
44 changes: 25 additions & 19 deletions src/staticdata_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -505,19 +505,17 @@ static void jl_collect_edges(jl_array_t *edges, jl_array_t *ext_targets, jl_arra
size_t max_valid = ~(size_t)0;
if (invokeTypes) {
assert(jl_is_method_instance(callee));
jl_methtable_t *mt = jl_method_get_table(((jl_method_instance_t*)callee)->def.method);
if ((jl_value_t*)mt == jl_nothing) {
callee_ids = NULL; // invalid
break;
}
else {
jl_method_t *m = ((jl_method_instance_t*)callee)->def.method;
matches = (jl_value_t*)m; // valid because there is no method replacement permitted
#ifndef NDEBUG
jl_methtable_t *mt = jl_method_get_table(m);
if ((jl_value_t*)mt != jl_nothing) {
matches = jl_gf_invoke_lookup_worlds(invokeTypes, (jl_value_t*)mt, world, &min_valid, &max_valid);
if (matches == jl_nothing) {
callee_ids = NULL; // invalid
break;
if (matches != jl_nothing) {
assert(m == ((jl_method_match_t*)matches)->method);
}
matches = (jl_value_t*)((jl_method_match_t*)matches)->method;
}
#endif
}
else {
if (jl_is_method_instance(callee)) {
Expand Down Expand Up @@ -855,19 +853,27 @@ static jl_array_t *jl_verify_edges(jl_array_t *targets, size_t minworld)
size_t max_valid = ~(size_t)0;
if (invokesig) {
assert(callee && "unsupported edge");
jl_methtable_t *mt = jl_method_get_table(((jl_method_instance_t*)callee)->def.method);
if ((jl_value_t*)mt == jl_nothing) {
max_valid = 0;
jl_method_t *m = ((jl_method_instance_t*)callee)->def.method;
if (jl_egal(invokesig, m->sig)) {
// the invoke match is `m` for `m->sig`, unless `m` is invalid
if (m->deleted_world < max_valid)
max_valid = 0;
}
else {
matches = jl_gf_invoke_lookup_worlds(invokesig, (jl_value_t*)mt, minworld, &min_valid, &max_valid);
if (matches == jl_nothing) {
max_valid = 0;
jl_methtable_t *mt = jl_method_get_table(m);
if ((jl_value_t*)mt == jl_nothing) {
max_valid = 0;
}
else {
matches = (jl_value_t*)((jl_method_match_t*)matches)->method;
if (matches != expected) {
max_valid = 0;
matches = jl_gf_invoke_lookup_worlds(invokesig, (jl_value_t*)mt, minworld, &min_valid, &max_valid);
if (matches == jl_nothing) {
max_valid = 0;
}
else {
matches = (jl_value_t*)((jl_method_match_t*)matches)->method;
if (matches != expected) {
max_valid = 0;
}
}
}
}
Expand Down
37 changes: 37 additions & 0 deletions test/worlds.jl
Original file line number Diff line number Diff line change
Expand Up @@ -419,3 +419,40 @@ ccall(:jl_debug_method_invalidation, Any, (Cint,), 0)
which(mc48954, (AbstractFloat, Int)),
"jl_method_table_insert"
]

# issue #50091 -- missing invoke edge affecting nospecialized dispatch
module ExceptionUnwrapping
@nospecialize
unwrap_exception(@nospecialize(e)) = e
unwrap_exception(e::Base.TaskFailedException) = e.task.exception
@noinline function _summarize_task_exceptions(io::IO, exc, prefix = nothing)
_summarize_exception((;prefix,), io, exc)
nothing
end
@noinline function _summarize_exception(kws, io::IO, e::TaskFailedException)
_summarize_task_exceptions(io, e.task, kws.prefix)
end
# This is the overload that prints the actual exception that occurred.
result = Bool[]
@noinline function _summarize_exception(kws, io::IO, @nospecialize(exc))
global result
push!(result, unwrap_exception(exc) === exc)
if unwrap_exception(exc) !== exc # something uninferrable
return _summarize_exception(kws, io, unwrap_exception(exc))
end
end
struct X; x; end
end
let e = ExceptionUnwrapping.X(nothing)
@test ExceptionUnwrapping.unwrap_exception(e) === e
ExceptionUnwrapping._summarize_task_exceptions(devnull, e)
@test ExceptionUnwrapping.result == [true]
empty!(ExceptionUnwrapping.result)
end
ExceptionUnwrapping.unwrap_exception(e::ExceptionUnwrapping.X) = e.x
let e = ExceptionUnwrapping.X(nothing)
@test !(ExceptionUnwrapping.unwrap_exception(e) === e)
ExceptionUnwrapping._summarize_task_exceptions(devnull, e)
@test ExceptionUnwrapping.result == [false, true]
empty!(ExceptionUnwrapping.result)
end

0 comments on commit 210fb22

Please sign in to comment.