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

gf: add support for invalidating invoke edges #48954

Merged
merged 1 commit into from
Mar 9, 2023
Merged

gf: add support for invalidating invoke edges #48954

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
109 changes: 58 additions & 51 deletions src/gf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1817,6 +1817,36 @@ static int jl_type_intersection2(jl_value_t *t1, jl_value_t *t2, jl_value_t **is
return 1;
}

enum morespec_options {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if morespec -> morespecific would be worth the extra Chars. Also, what about morespecifc_true/morespecific_false/morespecific_unknown?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really want to deal with the code churn to rename an existing flag now. It is only used locally anyways.

morespec_unknown,
morespec_isnot,
morespec_is
};

// check if `type` is replacing `m` with an ambiguity here, given other methods in `d` that already match it
// precondition: type is not more specific than `m`
static int is_replacing(jl_value_t *type, jl_method_t *m, jl_method_t *const *d, size_t n, jl_value_t *isect, jl_value_t *isect2, char *morespec)
{
size_t k;
for (k = 0; k < n; k++) {
jl_method_t *m2 = d[k];
// see if m2 also fully covered this intersection
if (m == m2 || !(jl_subtype(isect, m2->sig) || (isect2 && jl_subtype(isect2, m2->sig))))
continue;
if (morespec[k] == (char)morespec_unknown)
morespec[k] = (char)(jl_type_morespecific(m2->sig, type) ? morespec_is : morespec_isnot);
if (morespec[k] == (char)morespec_is)
// not actually shadowing this--m2 will still be better
return 0;
// since m2 was also a previous match over isect,
// see if m was also previously dominant over all m2
if (!jl_type_morespecific(m->sig, m2->sig))
// m and m2 were previously ambiguous over the full intersection of mi with type, and will still be ambiguous with type
return 0;
}
return 1;
}

JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method, jl_tupletype_t *simpletype)
{
JL_TIMING(ADD_METHOD);
Expand Down Expand Up @@ -1851,7 +1881,7 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
oldvalue = get_intersect_matches(jl_atomic_load_relaxed(&mt->defs), newentry);

int invalidated = 0;
jl_method_t **d;
jl_method_t *const *d;
size_t j, n;
if (oldvalue == NULL) {
d = NULL;
Expand Down Expand Up @@ -1880,6 +1910,7 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
// -> less specific or ambiguous with any one of them: can ignore the missing edge (not missing)
// -> some may have been ambiguous: still are
// -> some may have been called: they may be partly replaced (will be detected in the loop later)
// c.f. `is_replacing`, which is a similar query, but with an existing method match to compare against
missing = 1;
size_t j;
for (j = 0; j < n; j++) {
Expand Down Expand Up @@ -1914,11 +1945,6 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
}
if (oldvalue) {
oldmi = jl_alloc_vec_any(0);
enum morespec_options {
morespec_unknown,
morespec_isnot,
morespec_is
};
char *morespec = (char*)alloca(n);
memset(morespec, morespec_unknown, n);
for (j = 0; j < n; j++) {
Expand All @@ -1935,6 +1961,11 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
continue;
isect3 = jl_type_intersection(m->sig, (jl_value_t*)mi->specTypes);
if (jl_type_intersection2(type, isect3, &isect, &isect2)) {
// TODO: this only checks pair-wise for ambiguities, but the ambiguities could arise from the interaction of multiple methods
// and thus might miss a case where we introduce an ambiguity between two existing methods
Copy link
Member

Choose a reason for hiding this comment

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

I don't doubt this is true, but is there a simple example of how this could happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

Simple? Probably not haha. The current canonical example in one argument looks something like this, where 1 and 2 are ambiguous, and 3 is ambiguous with 1+2 but neither 1 nor 2:

module Ambig8
# complex / unsorted(-able) ambiguities
f(::Union{typeof(pi), Integer}) =  1
f(::Union{AbstractIrrational, Int}) =  2
f(::Signed) = 3
end

This is only ambiguous over f(::Int), but it causes the sort order for f(::Signed) to be somewhat ambiguous too (since f(::Int) is one subtype of that). This is a simple case though, since it is mostly just subtyping relations, and subtyping is transitive.

A more complicated case (just invented here, never seen before!), might look like the below. Note how no 2 methods are ambiguous (there always exists a parameter of the union that distinguishes them), but that among the 3 of them, there exists a circularity of specificity such that none is uniformly dominant for f(::Int) over the other 2.

julia> f(::Union{Int, UInt, AbstractIrrational}) = 1
f (generic function with 1 method)

julia> f(::Union{Int, Float64, Unsigned}) = 2
f (generic function with 2 methods)

julia> f(::Union{Int, typeof(pi), AbstractFloat}) = 3
f (generic function with 3 methods)

julia> Base.isambiguous(methods(f)[1], methods(f)[2])
false

julia> Base.isambiguous(methods(f)[1], methods(f)[3])
false

julia> Base.isambiguous(methods(f)[2], methods(f)[3])
false

julia> f(1)
ERROR: MethodError: f(::Int64) is ambiguous.

Candidates:
  f(::Union{Float64, Int64, Unsigned})
    @ Main REPL[2]:1
  f(::Union{Irrational{:π}, Int64, AbstractFloat})
    @ Main REPL[3]:1
  f(::Union{Int64, UInt64, AbstractIrrational})
    @ Main REPL[1]:1

Possible fix, define
  f(::Int64)

Stacktrace:
 [1] top-level scope
   @ REPL[7]:1

Copy link
Member

Choose a reason for hiding this comment

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

Genius. And sad that it's so complicated (but no one said subtyping was easy).

// We could instead work to sort this into 3 groups `morespecific .. ambiguous .. lesspecific`, with `type` in ambiguous,
// such that everything in `morespecific` dominates everything in `ambiguous`, and everything in `ambiguous` dominates everything in `lessspecific`
// And then compute where each isect falls, and whether it changed group--necessitating invalidation--or not.
if (morespec[j] == (char)morespec_unknown)
morespec[j] = (char)(jl_type_morespecific(m->sig, type) ? morespec_is : morespec_isnot);
if (morespec[j] == (char)morespec_is)
Expand All @@ -1943,62 +1974,38 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
if (ambig == morespec_unknown)
ambig = jl_type_morespecific(type, m->sig) ? morespec_is : morespec_isnot;
// replacing a method--see if this really was the selected method previously
// over the intersection
if (ambig == morespec_isnot) {
size_t k;
for (k = 0; k < n; k++) {
jl_method_t *m2 = d[k];
if (m == m2 || !(jl_subtype(isect, m2->sig) || (isect && jl_subtype(isect, m2->sig))))
continue;
if (morespec[k] == (char)morespec_unknown)
morespec[k] = (char)(jl_type_morespecific(m2->sig, type) ? morespec_is : morespec_isnot);
if (morespec[k] == (char)morespec_is)
// not actually shadowing this--m2 will still be better
break;
// since m2 was also a previous match over isect,
// see if m was also previously dominant over all m2
if (!jl_type_morespecific(m->sig, m2->sig))
break;
}
if (k != n)
continue;
}
// Before deciding whether to invalidate `mi`, check each backedge for `invoke`s
if (mi->backedges) {
jl_array_t *backedges = mi->backedges;
// over the intersection (not ambiguous) and the new method will be selected now (morespec_is)
int replaced_dispatch = ambig == morespec_is || is_replacing(type, m, d, n, isect, isect2, morespec);
// found that this specialization dispatch got replaced by m
// call invalidate_backedges(&do_nothing_with_codeinst, mi, max_world, "jl_method_table_insert");
// but ignore invoke-type edges
jl_array_t *backedges = mi->backedges;
if (backedges) {
size_t ib = 0, insb = 0, nb = jl_array_len(backedges);
jl_value_t *invokeTypes;
jl_method_instance_t *caller;
while (ib < nb) {
ib = get_next_edge(backedges, ib, &invokeTypes, &caller);
if (!invokeTypes) {
// ordinary dispatch, invalidate
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) && (ambig == morespec_is || is_replacing(type, m, d, n, invokeTypes, NULL, morespec));
}
else {
replaced_edge = replaced_dispatch;
}
if (replaced_edge) {
invalidate_method_instance(&do_nothing_with_codeinst, caller, max_world, 1);
invalidated = 1;
} else {
// invoke-dispatch, check invokeTypes for validity
struct jl_typemap_assoc search = {invokeTypes, method->primary_world, NULL, 0, ~(size_t)0};
oldentry = jl_typemap_assoc_by_type(jl_atomic_load_relaxed(&mt->defs), &search, /*offs*/0, /*subtype*/0);
if (oldentry && oldentry->func.method == mi->def.method) {
// We can safely keep this method
jl_array_ptr_set(backedges, insb++, invokeTypes);
jl_array_ptr_set(backedges, insb++, caller);
} else {
invalidate_method_instance(&do_nothing_with_codeinst, caller, max_world, 1);
invalidated = 1;
}
}
else {
insb = set_next_edge(backedges, insb, invokeTypes, caller);
}
}
jl_array_del_end(backedges, nb - insb);
}
if (!mi->backedges || jl_array_len(mi->backedges) == 0) {
jl_array_ptr_1d_push(oldmi, (jl_value_t*)mi);
invalidate_external(mi, max_world);
if (mi->backedges) {
invalidated = 1;
invalidate_backedges(&do_nothing_with_codeinst, mi, max_world, "jl_method_table_insert");
}
}
jl_array_ptr_1d_push(oldmi, (jl_value_t*)mi);
invalidate_external(mi, max_world);
}
}
}
Expand Down