Skip to content

Commit

Permalink
follow up "[NFC] minor refactorings on gf.c & codegen.cpp (#50645)" (#…
Browse files Browse the repository at this point in the history
…50748)

This follows up commit
19f6926.
As discussed in PR, the changes in gf.c were not NFC and may lead to
performance problems, so should be reverted.
Also added more detailed comments on the codegen.cpp change.
  • Loading branch information
aviatesk authored Aug 2, 2023
1 parent bb4929d commit 3eddd17
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 25 deletions.
20 changes: 9 additions & 11 deletions src/codegen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8908,17 +8908,15 @@ jl_llvm_functions_t jl_emit_codeinst(
jl_gc_wb(codeinst, src);
}
}
else if (jl_is_method(def)) {// don't delete toplevel code
if (// and there is something to delete (test this before calling jl_ir_inlining_cost)
inferred != jl_nothing &&
// don't delete inlineable code, unless it is constant
(jl_atomic_load_relaxed(&codeinst->invoke) == jl_fptr_const_return_addr ||
(jl_ir_inlining_cost(inferred) == UINT16_MAX)) &&
// don't delete code when generating a precompile file
!(params.imaging || jl_options.incremental)) {
// if not inlineable, code won't be needed again
jl_atomic_store_release(&codeinst->inferred, jl_nothing);
}
// delete non-inlineable code, since it won't be needed again
// because we already emitted LLVM code from it and the native
// Julia-level optimization will never need to see it
else if (jl_is_method(def) && // don't delete toplevel code
inferred != jl_nothing && // and there is something to delete (test this before calling jl_ir_inlining_cost)
((jl_ir_inlining_cost(inferred) == UINT16_MAX) || // don't delete inlineable code
jl_atomic_load_relaxed(&codeinst->invoke) == jl_fptr_const_return_addr) && // unless it is constant
!(params.imaging || jl_options.incremental)) { // don't delete code when generating a precompile file
jl_atomic_store_release(&codeinst->inferred, jl_nothing);
}
}
}
Expand Down
29 changes: 15 additions & 14 deletions src/gf.c
Original file line number Diff line number Diff line change
Expand Up @@ -2316,10 +2316,8 @@ jl_code_instance_t *jl_method_compiled(jl_method_instance_t *mi, size_t world)
jl_code_instance_t *codeinst = jl_atomic_load_relaxed(&mi->cache);
while (codeinst) {
if (codeinst->min_world <= world && world <= codeinst->max_world) {
jl_callptr_t invoke = jl_atomic_load_acquire(&codeinst->invoke);
if (invoke != NULL) {
if (jl_atomic_load_relaxed(&codeinst->invoke) != NULL)
return codeinst;
}
}
codeinst = jl_atomic_load_relaxed(&codeinst->next);
}
Expand Down Expand Up @@ -2861,19 +2859,20 @@ STATIC_INLINE jl_value_t *verify_type(jl_value_t *v) JL_NOTSAFEPOINT
return v;
}

STATIC_INLINE jl_value_t *invoke_codeinst(jl_value_t *F, jl_value_t **args, uint32_t nargs, jl_code_instance_t *codeinst)
{
jl_callptr_t invoke = jl_atomic_load_acquire(&codeinst->invoke);
jl_value_t *res = invoke(F, args, nargs, codeinst);
return verify_type(res);
}

STATIC_INLINE jl_value_t *_jl_invoke(jl_value_t *F, jl_value_t **args, uint32_t nargs, jl_method_instance_t *mfunc, size_t world)
{
// manually inlined copy of jl_method_compiled
jl_code_instance_t *codeinst = jl_method_compiled(mfunc, world);
if (codeinst)
return invoke_codeinst(F, args, nargs, codeinst);
jl_code_instance_t *codeinst = jl_atomic_load_relaxed(&mfunc->cache);
while (codeinst) {
if (codeinst->min_world <= world && world <= codeinst->max_world) {
jl_callptr_t invoke = jl_atomic_load_acquire(&codeinst->invoke);
if (invoke != NULL) {
jl_value_t *res = invoke(F, args, nargs, codeinst);
return verify_type(res);
}
}
codeinst = jl_atomic_load_relaxed(&codeinst->next);
}
int64_t last_alloc = jl_options.malloc_log ? jl_gc_diff_total_bytes() : 0;
int last_errno = errno;
#ifdef _OS_WINDOWS_
Expand All @@ -2886,7 +2885,9 @@ STATIC_INLINE jl_value_t *_jl_invoke(jl_value_t *F, jl_value_t **args, uint32_t
errno = last_errno;
if (jl_options.malloc_log)
jl_gc_sync_total_bytes(last_alloc); // discard allocation count from compilation
return invoke_codeinst(F, args, nargs, codeinst);
jl_callptr_t invoke = jl_atomic_load_acquire(&codeinst->invoke);
jl_value_t *res = invoke(F, args, nargs, codeinst);
return verify_type(res);
}

JL_DLLEXPORT jl_value_t *jl_invoke(jl_value_t *F, jl_value_t **args, uint32_t nargs, jl_method_instance_t *mfunc)
Expand Down

2 comments on commit 3eddd17

@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.