codegen: reduce recursion in cfunction generation (#56806)

vtjnash · web-flow · commit cea6a9a6b7a6 · 2024-12-12T16:19:43.000-05:00
The regular code-path for this was only missing the age_ok handling, so
add in that handling so we can delete the custom path here for the test
and some of the brokenness that implied.
diff --git a/src/aotcompile.cpp b/src/aotcompile.cpp
@@ -505,7 +505,7 @@ static void compile_workqueue(jl_codegen_params_t &params, egal_set &method_root
             size_t nrealargs = jl_nparams(mi->specTypes); // number of actual arguments being passed
             bool is_opaque_closure = jl_is_method(mi->def.value) && mi->def.method->is_for_opaque_closure;
             // TODO: maybe this can be cached in codeinst->specfptr?
-            emit_specsig_to_fptr1(proto.decl, proto.cc, proto.return_roots, mi->specTypes, codeinst->rettype, is_opaque_closure, nrealargs, params, pinvoke, 0, 0);
+            emit_specsig_to_fptr1(proto.decl, proto.cc, proto.return_roots, mi->specTypes, codeinst->rettype, is_opaque_closure, nrealargs, params, pinvoke);
             preal_decl = ""; // no need to fixup the name
         }
         if (!preal_decl.empty()) {
diff --git a/src/cgutils.cpp b/src/cgutils.cpp
@@ -2279,7 +2279,7 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
         const jl_cgval_t argv[3] = { cmp, lhs, rhs };
         jl_cgval_t ret;
         if (modifyop) {
-            ret = emit_invoke(ctx, *modifyop, argv, 3, (jl_value_t*)jl_any_type);
+            ret = emit_invoke(ctx, *modifyop, argv, 3, (jl_value_t*)jl_any_type, nullptr);
         }
         else {
             if (trim_may_error(ctx.params->trim)) {
@@ -4018,7 +4018,7 @@ static jl_cgval_t union_store(jl_codectx_t &ctx,
                 emit_lockstate_value(ctx, needlock, false);
             const jl_cgval_t argv[3] = { cmp, oldval, rhs };
             if (modifyop) {
-                rhs = emit_invoke(ctx, *modifyop, argv, 3, (jl_value_t*)jl_any_type);
+                rhs = emit_invoke(ctx, *modifyop, argv, 3, (jl_value_t*)jl_any_type, nullptr);
             }
             else {
                 if (trim_may_error(ctx.params->trim)) {
diff --git a/src/codegen.cpp b/src/codegen.cpp
diff --git a/src/gf.c b/src/gf.c
@@ -3084,7 +3084,7 @@ JL_DLLEXPORT jl_method_instance_t *jl_method_match_to_mi(jl_method_match_t *matc
 }
 
 // compile-time method lookup
-jl_method_instance_t *jl_get_specialization1(jl_tupletype_t *types JL_PROPAGATES_ROOT, size_t world, size_t *min_valid, size_t *max_valid, int mt_cache)
+jl_method_instance_t *jl_get_specialization1(jl_tupletype_t *types JL_PROPAGATES_ROOT, size_t world, int mt_cache)
 {
     if (jl_has_free_typevars((jl_value_t*)types))
         return NULL; // don't poison the cache due to a malformed query
@@ -3096,10 +3096,6 @@ jl_method_instance_t *jl_get_specialization1(jl_tupletype_t *types JL_PROPAGATES
     size_t max_valid2 = ~(size_t)0;
     int ambig = 0;
     jl_value_t *matches = jl_matching_methods(types, jl_nothing, 1, 1, world, &min_valid2, &max_valid2, &ambig);
-    if (*min_valid < min_valid2)
-        *min_valid = min_valid2;
-    if (*max_valid > max_valid2)
-        *max_valid = max_valid2;
     if (matches == jl_nothing || jl_array_nrows(matches) != 1 || ambig)
         return NULL;
     JL_GC_PUSH1(&matches);
diff --git a/src/jitlayers.cpp b/src/jitlayers.cpp
@@ -382,7 +382,7 @@ static int jl_analyze_workqueue(jl_code_instance_t *callee, jl_codegen_params_t
                 jl_method_instance_t *mi = codeinst->def;
                 size_t nrealargs = jl_nparams(mi->specTypes); // number of actual arguments being passed
                 bool is_opaque_closure = jl_is_method(mi->def.value) && mi->def.method->is_for_opaque_closure;
-                emit_specsig_to_fptr1(proto.decl, proto.cc, proto.return_roots, mi->specTypes, codeinst->rettype, is_opaque_closure, nrealargs, params, pinvoke, 0, 0);
+                emit_specsig_to_fptr1(proto.decl, proto.cc, proto.return_roots, mi->specTypes, codeinst->rettype, is_opaque_closure, nrealargs, params, pinvoke);
                 jl_gc_unsafe_leave(ct->ptls, gc_state);
                 preal_decl = ""; // no need to fixup the name
             }
@@ -713,7 +713,7 @@ static void jl_emit_codeinst_to_jit(
     int waiting = jl_analyze_workqueue(codeinst, params);
     if (waiting) {
         auto release = std::move(params.tsctx_lock); // unlock again before moving from it
-        incompletemodules.insert(std::pair(codeinst, std::make_tuple(std::move(params), waiting)));
+        incompletemodules.try_emplace(codeinst, std::move(params), waiting);
     }
     else {
         finish_params(result_m.getModuleUnlocked(), params);
@@ -760,7 +760,7 @@ static void _jl_compile_codeinst(
 }
 
 
-const char *jl_generate_ccallable(LLVMOrcThreadSafeModuleRef llvmmod, void *sysimg_handle, jl_value_t *declrt, jl_value_t *sigt, jl_codegen_params_t &params);
+const char *jl_generate_ccallable(Module *llvmmod, void *sysimg_handle, jl_value_t *declrt, jl_value_t *sigt, jl_codegen_params_t &params);
 
 // compile a C-callable alias
 extern "C" JL_DLLEXPORT_CODEGEN
@@ -774,45 +774,68 @@ int jl_compile_extern_c_impl(LLVMOrcThreadSafeModuleRef llvmmod, void *p, void *
     uint8_t measure_compile_time_enabled = jl_atomic_load_relaxed(&jl_measure_compile_time_enabled);
     if (measure_compile_time_enabled)
         compiler_start_time = jl_hrtime();
+    jl_codegen_params_t *pparams = (jl_codegen_params_t*)p;
+    DataLayout DL = pparams ? pparams->DL : jl_ExecutionEngine->getDataLayout();
+    Triple TargetTriple = pparams ? pparams->TargetTriple : jl_ExecutionEngine->getTargetTriple();
     orc::ThreadSafeContext ctx;
     auto into = unwrap(llvmmod);
-    jl_codegen_params_t *pparams = (jl_codegen_params_t*)p;
     orc::ThreadSafeModule backing;
+    bool success = true;
+    const char *name = "";
+    SmallVector<jl_code_instance_t*,0> dependencies;
     if (into == NULL) {
-        if (!pparams) {
-            ctx = jl_ExecutionEngine->makeContext();
-        }
-        backing = jl_create_ts_module("cextern", pparams ? pparams->tsctx : ctx,  pparams ? pparams->DL : jl_ExecutionEngine->getDataLayout(), pparams ? pparams->TargetTriple : jl_ExecutionEngine->getTargetTriple());
+        ctx = pparams ? pparams->tsctx : jl_ExecutionEngine->makeContext();
+        backing = jl_create_ts_module("cextern", ctx, DL, TargetTriple);
         into = &backing;
     }
-    bool success = true;
-    {
-        auto Lock = into->getContext().getLock();
-        Module *M = into->getModuleUnlocked();
-        jl_codegen_params_t params(into->getContext(), M->getDataLayout(), Triple(M->getTargetTriple()));
-        params.imaging_mode = imaging_default();
+    { // params scope
+        jl_codegen_params_t params(into->getContext(), DL, TargetTriple);
         if (pparams == NULL) {
-            M->getContext().setDiscardValueNames(true);
+            params.cache = p == NULL;
+            params.imaging_mode = imaging_default();
+            params.tsctx.getContext()->setDiscardValueNames(true);
             pparams = &params;
         }
-        assert(pparams->tsctx.getContext() == into->getContext().getContext());
-        const char *name = jl_generate_ccallable(wrap(into), sysimg, declrt, sigt, *pparams);
-        if (!sysimg) {
-            jl_unique_gcsafe_lock lock(extern_c_lock);
-            if (jl_ExecutionEngine->getGlobalValueAddress(name)) {
-                success = false;
+        Module &M = *into->getModuleUnlocked();
+        assert(pparams->tsctx.getContext() == &M.getContext());
+        name = jl_generate_ccallable(&M, sysimg, declrt, sigt, *pparams);
+        if (!sysimg && !p) {
+            { // drop lock to keep analyzer happy (since it doesn't know we have the only reference to it)
+                auto release = std::move(params.tsctx_lock);
             }
-            if (success && p == NULL) {
-                jl_jit_globals(params.global_targets);
-                assert(params.workqueue.empty());
-                if (params._shared_module) {
-                    jl_ExecutionEngine->optimizeDLSyms(*params._shared_module); // safepoint
-                    jl_ExecutionEngine->addModule(orc::ThreadSafeModule(std::move(params._shared_module), params.tsctx));
+            { // lock scope
+                jl_unique_gcsafe_lock lock(extern_c_lock);
+                if (jl_ExecutionEngine->getGlobalValueAddress(name))
+                    success = false;
+            }
+            params.tsctx_lock = params.tsctx.getLock(); // re-acquire lock
+            if (success && params.cache) {
+                for (auto &it : params.workqueue) {
+                    jl_code_instance_t *codeinst = it.first;
+                    JL_GC_PROMISE_ROOTED(codeinst);
+                    dependencies.push_back(codeinst);
+                    recursive_compile_graph(codeinst, nullptr);
                 }
+                jl_analyze_workqueue(nullptr, params, true);
+                assert(params.workqueue.empty());
+                finish_params(&M, params);
             }
-            if (success && llvmmod == NULL) {
-                jl_ExecutionEngine->optimizeDLSyms(*M); // safepoint
-                jl_ExecutionEngine->addModule(std::move(*into));
+        }
+        pparams = nullptr;
+    }
+    if (!sysimg && success && llvmmod == NULL) {
+        { // lock scope
+            jl_unique_gcsafe_lock lock(extern_c_lock);
+            if (!jl_ExecutionEngine->getGlobalValueAddress(name)) {
+                for (auto dep : dependencies)
+                    jl_compile_codeinst_now(dep);
+                {
+                    auto Lock = backing.getContext().getLock();
+                    jl_ExecutionEngine->optimizeDLSyms(*backing.getModuleUnlocked()); // safepoint
+                }
+                jl_ExecutionEngine->addModule(std::move(backing));
+                success = jl_ExecutionEngine->getGlobalValueAddress(name);
+                assert(success);
             }
         }
     }
diff --git a/src/jitlayers.h b/src/jitlayers.h
@@ -260,7 +260,7 @@ struct jl_codegen_params_t {
     bool external_linkage = false;
     bool imaging_mode;
     bool use_swiftcc = true;
-    jl_codegen_params_t(orc::ThreadSafeContext ctx, DataLayout DL, Triple triple)
+    jl_codegen_params_t(orc::ThreadSafeContext ctx, DataLayout DL, Triple triple) JL_NOTSAFEPOINT  JL_NOTSAFEPOINT_ENTER
       : tsctx(std::move(ctx)),
         tsctx_lock(tsctx.getLock()),
         DL(std::move(DL)),
@@ -271,6 +271,8 @@ struct jl_codegen_params_t {
         if (TargetTriple.isRISCV())
             use_swiftcc = false;
     }
+    jl_codegen_params_t(jl_codegen_params_t &&) JL_NOTSAFEPOINT = default;
+    ~jl_codegen_params_t() JL_NOTSAFEPOINT JL_NOTSAFEPOINT_LEAVE = default;
 };
 
 jl_llvm_functions_t jl_emit_code(
@@ -299,8 +301,7 @@ void emit_specsig_to_fptr1(
         jl_value_t *calltype, jl_value_t *rettype, bool is_for_opaque_closure,
         size_t nargs,
         jl_codegen_params_t &params,
-        Function *target,
-        size_t min_world, size_t max_world) JL_NOTSAFEPOINT;
+        Function *target) JL_NOTSAFEPOINT;
 Function *get_or_emit_fptr1(StringRef Name, Module *M) JL_NOTSAFEPOINT;
 void jl_init_function(Function *F, const Triple &TT) JL_NOTSAFEPOINT;
 
diff --git a/src/julia_internal.h b/src/julia_internal.h
@@ -1195,7 +1195,7 @@ _Atomic(jl_value_t*) *jl_table_peek_bp(jl_genericmemory_t *a, jl_value_t *key) J
 JL_DLLEXPORT jl_method_t *jl_new_method_uninit(jl_module_t*);
 
 JL_DLLEXPORT jl_methtable_t *jl_new_method_table(jl_sym_t *name, jl_module_t *module);
-JL_DLLEXPORT jl_method_instance_t *jl_get_specialization1(jl_tupletype_t *types, size_t world, size_t *min_valid, size_t *max_valid, int mt_cache);
+JL_DLLEXPORT jl_method_instance_t *jl_get_specialization1(jl_tupletype_t *types, size_t world, int mt_cache);
 jl_method_instance_t *jl_get_specialized(jl_method_t *m, jl_value_t *types, jl_svec_t *sp);
 JL_DLLEXPORT jl_value_t *jl_rettype_inferred(jl_value_t *owner, jl_method_instance_t *li JL_PROPAGATES_ROOT, size_t min_world, size_t max_world);
 JL_DLLEXPORT jl_value_t *jl_rettype_inferred_native(jl_method_instance_t *mi, size_t min_world, size_t max_world) JL_NOTSAFEPOINT;
diff --git a/src/precompile_utils.c b/src/precompile_utils.c
@@ -264,10 +264,8 @@ static void *jl_precompile_(jl_array_t *m, int external_linkage)
         jl_value_t *item = jl_array_ptr_ref(m, i);
         if (jl_is_method_instance(item)) {
             mi = (jl_method_instance_t*)item;
-            size_t min_world = 0;
-            size_t max_world = ~(size_t)0;
             if (mi != jl_atomic_load_relaxed(&mi->def.method->unspecialized) && !jl_isa_compileable_sig((jl_tupletype_t*)mi->specTypes, mi->sparam_vals, mi->def.method))
-                mi = jl_get_specialization1((jl_tupletype_t*)mi->specTypes, jl_atomic_load_acquire(&jl_world_counter), &min_world, &max_world, 0);
+                mi = jl_get_specialization1((jl_tupletype_t*)mi->specTypes, jl_atomic_load_acquire(&jl_world_counter), 0);
             if (mi)
                 jl_array_ptr_1d_push(m2, (jl_value_t*)mi);
         }
diff --git a/test/llvmcall.jl b/test/llvmcall.jl
@@ -157,7 +157,7 @@ module ObjLoadTest
         nothing
     end
     @test_throws(ErrorException("@ccallable was already defined for this method name"),
-                 @eval @ccallable Cvoid jl_the_callback(not_the_method::Int) = "other")
+            @eval @ccallable String jl_the_callback(not_the_method::Int) = "other")
     # Make sure everything up until here gets compiled
     @test jl_the_callback() === nothing
     @test jl_the_callback(1) == "other"

Original file line number	Diff line number	Diff line change
`@@ -505,7 +505,7 @@ static void compile_workqueue(jl_codegen_params_t &params, egal_set &method_root`
`505`	`505`	`size_t nrealargs = jl_nparams(mi->specTypes); // number of actual arguments being passed`
`506`	`506`	`bool is_opaque_closure = jl_is_method(mi->def.value) && mi->def.method->is_for_opaque_closure;`
`507`	`507`	`// TODO: maybe this can be cached in codeinst->specfptr?`
`508`		`- emit_specsig_to_fptr1(proto.decl, proto.cc, proto.return_roots, mi->specTypes, codeinst->rettype, is_opaque_closure, nrealargs, params, pinvoke, 0, 0);`
	`508`	`+ emit_specsig_to_fptr1(proto.decl, proto.cc, proto.return_roots, mi->specTypes, codeinst->rettype, is_opaque_closure, nrealargs, params, pinvoke);`
`509`	`509`	`preal_decl = ""; // no need to fixup the name`
`510`	`510`	`}`
`511`	`511`	`if (!preal_decl.empty()) {`