Skip to content

staticdata: Remove new_ext_cis list #58133

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.

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

topolarity
Copy link
Member

@topolarity topolarity commented Apr 16, 2025

As a root for compilation, this list is pretty much redundant with the external CI's that we will discover along the forward call edges in inference. As a filter for code emission, it is mostly redundant with specsigflags & 0b100, which is the only thing jl_emit_native respects as a filter right now anyway.

The other purpose of this list was to track what needs to be inserted into cache / methods loaded from another pkgimage (i.e. CI's for external methods), which is now tracked by a new specsigflags & 0b1000 flag.

This means that the list of "newly-inferred" CI's / MI's (as defined by JL_MI_FLAGS_MASK_PRECOMPILED / jl_push_newly_inferred / jl_tag_newly_inferred_enable) is purely a "best-effort" list - the runtime itself no longer depends on it.

TODO: The last remaining use for this list is to track which MI/CI's for external methods were triggered via precompile(...). Otherwise these will not make it into the queue.

As a root for compilation, this list is pretty much redundant with
the external CI's that we will discover along the forward-edges via
inference. As a filter for code emission, it is also redundant with
`specsigflags & 0b100`

Since c136b7e, we have already tracked whether a CodeInstance is
"newly-inferred" based on its specsigflags, which is also the only
thing respected by `jl_emit_native` to filter externally-provided
code from a pkgimage. Technically this list was slightly more fine-
grained since loading would interact with it - but it looks like
we've been ignoring that for some time.

Finally, this keeps track of whether the CodeInstance is targeting
an external method (i.e. needs to be inserted into cache / methods
loaded from another pkgimage) via a new `specsigflags & 0b1000` flag
@topolarity topolarity requested a review from vtjnash April 16, 2025 00:24
@topolarity
Copy link
Member Author

cc @timholy, to make sure this best-effort definition for "newly-inferred" / precompile-tagged is OK
(if not, let's try to find a precise definition)

@topolarity
Copy link
Member Author

topolarity commented Apr 16, 2025

Oof. This does affect pkgimages significantly after all.

Compiling with JULIA_CPU_TARGET="generic;sandybridge,-xsaveopt,clone_all;haswell,-rdrnd,base(1);x86-64-v4,-rdrnd,base(1)", it looks like Pkg's image shrunk by about 10 MB (74 MB -> 65 MB) and basecompiler.so is down about 20 MB (78 MB -> 57 MB). A number of precompilation tests also got affected.

edit: Ah, I think I see the problem. precompile(...) statements for external methods are being dropped, since they are not discovered by this walk any more:

if (!suppress_precompile) {
size_t i, n = jl_array_nrows(worklist);
for (i = 0; i < n; i++) {
jl_module_t *mod = (jl_module_t*)jl_array_ptr_ref(worklist, i);
assert(jl_is_module(mod));
foreach_mtable_in_module(mod, precompile_enq_all_specializations_, m);
}
n = jl_array_nrows(extext_methods);
for (i = 0; i < n; i++) {
jl_method_t *method = (jl_method_t*)jl_array_ptr_ref(extext_methods, i);
assert(jl_is_method(method));
jl_value_t *specializations = jl_atomic_load_relaxed(&method->specializations);
if (!jl_is_svec(specializations)) {
precompile_enq_specialization_((jl_method_instance_t*)specializations, m);
}
else {
size_t j, l = jl_svec_len(specializations);
for (j = 0; j < l; j++) {
jl_value_t *mi = jl_svecref(specializations, j);
if (mi != jl_nothing)
precompile_enq_specialization_((jl_method_instance_t*)mi, m);
}
}
}
}

@topolarity topolarity marked this pull request as draft April 16, 2025 03:52
@vchuravy
Copy link
Member

Yeah it's original purpose was to describe the root set of external methods who have new CI for a package image. This includes CI from a alternative abstract interpreter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants