From 3dcc07ce0007e6222d6996fb4e340e3423102937 Mon Sep 17 00:00:00 2001 From: Shuhei Kadowaki Date: Thu, 11 Jan 2024 17:52:56 +0900 Subject: [PATCH 1/8] effects: fix #52843, account for mutable values directly embedded to IR Fixes another variant of #52531. --- base/compiler/abstractinterpretation.jl | 14 +++++++------- test/compiler/effects.jl | 6 ++++++ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/base/compiler/abstractinterpretation.jl b/base/compiler/abstractinterpretation.jl index 3701fa8cf9cae9..5c27edea6d6861 100644 --- a/base/compiler/abstractinterpretation.jl +++ b/base/compiler/abstractinterpretation.jl @@ -2310,11 +2310,7 @@ function abstract_eval_cfunction(interp::AbstractInterpreter, e::Expr, vtypes::U end function abstract_eval_special_value(interp::AbstractInterpreter, @nospecialize(e), vtypes::Union{VarTable,Nothing}, sv::AbsIntState) - if isa(e, QuoteNode) - effects = Effects(EFFECTS_TOTAL; - inaccessiblememonly = is_mutation_free_argtype(typeof(e.value)) ? ALWAYS_TRUE : ALWAYS_FALSE) - return RTEffects(Const(e.value), Union{}, effects) - elseif isa(e, SSAValue) + if isa(e, SSAValue) return RTEffects(abstract_eval_ssavalue(e, sv), Union{}, EFFECTS_TOTAL) elseif isa(e, SlotNumber) if vtypes !== nothing @@ -2335,8 +2331,12 @@ function abstract_eval_special_value(interp::AbstractInterpreter, @nospecialize( elseif isa(e, GlobalRef) return abstract_eval_globalref(interp, e, sv) end - - return RTEffects(Const(e), Union{}, EFFECTS_TOTAL) + if isa(e, QuoteNode) + e = e.value + end + effects = Effects(EFFECTS_TOTAL; + inaccessiblememonly = is_mutation_free_argtype(typeof(e)) ? ALWAYS_TRUE : ALWAYS_FALSE) + return RTEffects(Const(e), Union{}, effects) end function abstract_eval_value_expr(interp::AbstractInterpreter, e::Expr, sv::AbsIntState) diff --git a/test/compiler/effects.jl b/test/compiler/effects.jl index 2c303f53356339..71eb50cd427608 100644 --- a/test/compiler/effects.jl +++ b/test/compiler/effects.jl @@ -1370,6 +1370,12 @@ top_52531(_) = (set_initialized52531!(true); nothing) top_52531(0) @test is_initialized52531() +const ref52843 = Ref{Int}() +@eval func52843() = ($ref52843[] = 1; nothing) +@test !Core.Compiler.is_foldable(Base.infer_effects(func52843)) +let; Base.Experimental.@force_compile; func52843(); end +@test ref52843[] == 1 + @test Core.Compiler.is_inaccessiblememonly(Base.infer_effects(identity∘identity, Tuple{Any})) @test Core.Compiler.is_inaccessiblememonly(Base.infer_effects(()->Vararg, Tuple{})) From c5d7b87a35b5beaef9d4d3aa53c0a2686f3445b9 Mon Sep 17 00:00:00 2001 From: Jishnu Bhattacharya Date: Thu, 11 Jan 2024 18:56:23 +0530 Subject: [PATCH 2/8] Fix variable name in scaling an `AbstractTriangular` with zero alpha (#52855) There is no `C` defined in these methods, so this branch used to error. --- stdlib/LinearAlgebra/src/triangular.jl | 16 ++++++++-------- stdlib/LinearAlgebra/test/triangular.jl | 5 +++++ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/stdlib/LinearAlgebra/src/triangular.jl b/stdlib/LinearAlgebra/src/triangular.jl index 4931e314d0d5c1..6e92b9f07dd840 100644 --- a/stdlib/LinearAlgebra/src/triangular.jl +++ b/stdlib/LinearAlgebra/src/triangular.jl @@ -545,7 +545,7 @@ end function _triscale!(A::UpperTriangular, B::UpperTriangular, c::Number, _add) n = checksize1(A, B) - iszero(_add.alpha) && return _rmul_or_fill!(C, _add.beta) + iszero(_add.alpha) && return _rmul_or_fill!(A, _add.beta) for j = 1:n for i = 1:j @inbounds _modify!(_add, B.data[i,j] * c, A.data, (i,j)) @@ -555,7 +555,7 @@ function _triscale!(A::UpperTriangular, B::UpperTriangular, c::Number, _add) end function _triscale!(A::UpperTriangular, c::Number, B::UpperTriangular, _add) n = checksize1(A, B) - iszero(_add.alpha) && return _rmul_or_fill!(C, _add.beta) + iszero(_add.alpha) && return _rmul_or_fill!(A, _add.beta) for j = 1:n for i = 1:j @inbounds _modify!(_add, c * B.data[i,j], A.data, (i,j)) @@ -565,7 +565,7 @@ function _triscale!(A::UpperTriangular, c::Number, B::UpperTriangular, _add) end function _triscale!(A::UpperOrUnitUpperTriangular, B::UnitUpperTriangular, c::Number, _add) n = checksize1(A, B) - iszero(_add.alpha) && return _rmul_or_fill!(C, _add.beta) + iszero(_add.alpha) && return _rmul_or_fill!(A, _add.beta) for j = 1:n @inbounds _modify!(_add, c, A, (j,j)) for i = 1:(j - 1) @@ -576,7 +576,7 @@ function _triscale!(A::UpperOrUnitUpperTriangular, B::UnitUpperTriangular, c::Nu end function _triscale!(A::UpperOrUnitUpperTriangular, c::Number, B::UnitUpperTriangular, _add) n = checksize1(A, B) - iszero(_add.alpha) && return _rmul_or_fill!(C, _add.beta) + iszero(_add.alpha) && return _rmul_or_fill!(A, _add.beta) for j = 1:n @inbounds _modify!(_add, c, A, (j,j)) for i = 1:(j - 1) @@ -587,7 +587,7 @@ function _triscale!(A::UpperOrUnitUpperTriangular, c::Number, B::UnitUpperTriang end function _triscale!(A::LowerTriangular, B::LowerTriangular, c::Number, _add) n = checksize1(A, B) - iszero(_add.alpha) && return _rmul_or_fill!(C, _add.beta) + iszero(_add.alpha) && return _rmul_or_fill!(A, _add.beta) for j = 1:n for i = j:n @inbounds _modify!(_add, B.data[i,j] * c, A.data, (i,j)) @@ -597,7 +597,7 @@ function _triscale!(A::LowerTriangular, B::LowerTriangular, c::Number, _add) end function _triscale!(A::LowerTriangular, c::Number, B::LowerTriangular, _add) n = checksize1(A, B) - iszero(_add.alpha) && return _rmul_or_fill!(C, _add.beta) + iszero(_add.alpha) && return _rmul_or_fill!(A, _add.beta) for j = 1:n for i = j:n @inbounds _modify!(_add, c * B.data[i,j], A.data, (i,j)) @@ -607,7 +607,7 @@ function _triscale!(A::LowerTriangular, c::Number, B::LowerTriangular, _add) end function _triscale!(A::LowerOrUnitLowerTriangular, B::UnitLowerTriangular, c::Number, _add) n = checksize1(A, B) - iszero(_add.alpha) && return _rmul_or_fill!(C, _add.beta) + iszero(_add.alpha) && return _rmul_or_fill!(A, _add.beta) for j = 1:n @inbounds _modify!(_add, c, A, (j,j)) for i = (j + 1):n @@ -618,7 +618,7 @@ function _triscale!(A::LowerOrUnitLowerTriangular, B::UnitLowerTriangular, c::Nu end function _triscale!(A::LowerOrUnitLowerTriangular, c::Number, B::UnitLowerTriangular, _add) n = checksize1(A, B) - iszero(_add.alpha) && return _rmul_or_fill!(C, _add.beta) + iszero(_add.alpha) && return _rmul_or_fill!(A, _add.beta) for j = 1:n @inbounds _modify!(_add, c, A, (j,j)) for i = (j + 1):n diff --git a/stdlib/LinearAlgebra/test/triangular.jl b/stdlib/LinearAlgebra/test/triangular.jl index dfe3f60d2a639b..7de1d10fe6d6c6 100644 --- a/stdlib/LinearAlgebra/test/triangular.jl +++ b/stdlib/LinearAlgebra/test/triangular.jl @@ -252,6 +252,11 @@ for elty1 in (Float32, Float64, BigFloat, ComplexF32, ComplexF64, Complex{BigFlo A2tmp = unitt(A1) mul!(A1tmp, cr, A2tmp) @test A1tmp == cr * A2tmp + + A1tmp .= A1 + @test mul!(A1tmp, A2tmp, cr, 0, 2) == 2A1 + A1tmp .= A1 + @test mul!(A1tmp, cr, A2tmp, 0, 2) == 2A1 else A1tmp = copy(A1) rmul!(A1tmp, ci) From 403a80a6f8d707359ca1aeab4ed9d1acfb49660c Mon Sep 17 00:00:00 2001 From: Lilith Orion Hafner Date: Fri, 12 Jan 2024 00:48:52 +0600 Subject: [PATCH 3/8] Precision of language in `names` docstring (#52744) --- base/reflection.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/base/reflection.jl b/base/reflection.jl index 0295657eea1fcc..67044350e6b25e 100644 --- a/base/reflection.jl +++ b/base/reflection.jl @@ -75,7 +75,7 @@ end """ names(x::Module; all::Bool = false, imported::Bool = false) -Get an array of the public names of a `Module`, excluding deprecated names. +Get a vector of the public names of a `Module`, excluding deprecated names. If `all` is true, then the list also includes non-public names defined in the module, deprecated names, and compiler-generated names. If `imported` is true, then names explicitly imported from other modules From 0fdd55b163ec0b7c73745d5395d12cff300be6fd Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Fri, 12 Jan 2024 04:04:19 +0100 Subject: [PATCH 4/8] Fix inconsistency between exct and nothrow effect (#52853) Fixes #52846. Different unreachability checks use different definitions for this and were inconsistenct, causing the assertion. Since the exct modeling isn't fully plubmed through all corners of the compiler yet, also change the caller code to force the nothrow effect to be authoritative for the time being. --- base/compiler/abstractinterpretation.jl | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/base/compiler/abstractinterpretation.jl b/base/compiler/abstractinterpretation.jl index 5c27edea6d6861..1c075fd4d8a257 100644 --- a/base/compiler/abstractinterpretation.jl +++ b/base/compiler/abstractinterpretation.jl @@ -2119,8 +2119,14 @@ function abstract_call_known(interp::AbstractInterpreter, @nospecialize(f), abstract_call_gf_by_type(interp, f, ArgInfo(nothing, T), si, atype, sv, max_methods) end pT = typevar_tfunc(𝕃ᵢ, n, lb_var, ub_var) - effects = builtin_effects(𝕃ᵢ, Core._typevar, Any[n, lb_var, ub_var], pT) - return CallMeta(pT, Any, effects, call.info) + typevar_argtypes = Any[n, lb_var, ub_var] + effects = builtin_effects(𝕃ᵢ, Core._typevar, typevar_argtypes, pT) + if effects.nothrow + exct = Union{} + else + exct = builtin_exct(𝕃ᵢ, Core._typevar, typevar_argtypes, pT) + end + return CallMeta(pT, exct, effects, call.info) elseif f === UnionAll call = abstract_call_gf_by_type(interp, f, ArgInfo(nothing, Any[Const(UnionAll), Any, Any]), si, Tuple{Type{UnionAll}, Any, Any}, sv, max_methods) return abstract_call_unionall(interp, argtypes, call) @@ -3288,10 +3294,12 @@ function typeinf_local(interp::AbstractInterpreter, frame::InferenceState) # Process non control-flow statements (; changes, rt, exct) = abstract_eval_basic_statement(interp, stmt, currstate, frame) - if exct !== Union{} - update_exc_bestguess!(interp, exct, frame) - end if !has_curr_ssaflag(frame, IR_FLAG_NOTHROW) + if exct !== Union{} + update_exc_bestguess!(interp, exct, frame) + # TODO: assert that these conditions match. For now, we assume the `nothrow` flag + # to be correct, but allow the exct to be an over-approximation. + end propagate_to_error_handler!(currstate, frame, 𝕃ᵢ) end if rt === Bottom From 9836f2d61dfaaac58e314636b1702b0f070571e8 Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Fri, 12 Jan 2024 15:18:08 +0100 Subject: [PATCH 5/8] staticdata: Some refactors for clarity (#52852) I was reading this code as part of looking into #52797. To recap, when we serialize objects for package images, we classify each method as either internal or external, depending on whether the method extends a function in the ambient set of modules ( system image, other previously loaded package images, etc) or whether it is private to the module we're currently serializing (in which case we call it internal). One of my primary confusions in reading this code was that the `new_specializations` object holds only external code instances in most of the code, but towards the end of loading, we used to also add any internal code instances that had dependencies on other external method instances in order to share the code instance validation code between the two cases. I don't think this design is that great. Conceptually, internal CodeInstances are already in the CI cache at the point that validation happens (their max_world is just set to 1, so they're not eligible to run). We do guard the cache insertion by a check whether a code instance already exists, which also catches this case, but I think it's cleaner to just not add any internal code instances to the list and instead simply re-activate the code instance in situ. Another issue with the old code that I didn't observe, but I think is possible in theory is that any CodeInstances that are not part of the cache hierarchy (e.g. because they were created by external abstract interpreters) should not accidentally get added to the cache hierarchy by this code path. I think this was possible in the old code, but I didn't observe it. With this change, `new_specializations` now always only holds external cis, so rename it appropriately. While we're at it, also do some other clarity changes. --- src/precompile_utils.c | 6 +-- src/staticdata.c | 98 +++++++++++++++++++++--------------------- src/staticdata_utils.c | 70 ++++++++++++++++++------------ test/precompile.jl | 4 +- 4 files changed, 97 insertions(+), 81 deletions(-) diff --git a/src/precompile_utils.c b/src/precompile_utils.c index 0203569f33c373..338c62b7e2d72d 100644 --- a/src/precompile_utils.c +++ b/src/precompile_utils.c @@ -279,7 +279,7 @@ static void *jl_precompile(int all) return native_code; } -static void *jl_precompile_worklist(jl_array_t *worklist, jl_array_t *extext_methods, jl_array_t *new_specializations) +static void *jl_precompile_worklist(jl_array_t *worklist, jl_array_t *extext_methods, jl_array_t *new_ext_cis) { if (!worklist) return NULL; @@ -310,9 +310,9 @@ static void *jl_precompile_worklist(jl_array_t *worklist, jl_array_t *extext_met } } } - n = jl_array_nrows(new_specializations); + n = jl_array_nrows(new_ext_cis); for (i = 0; i < n; i++) { - jl_code_instance_t *ci = (jl_code_instance_t*)jl_array_ptr_ref(new_specializations, i); + jl_code_instance_t *ci = (jl_code_instance_t*)jl_array_ptr_ref(new_ext_cis, i); precompile_enq_specialization_(ci->def, m); } void *native_code = jl_precompile_(m, 1); diff --git a/src/staticdata.c b/src/staticdata.c index 8ad4a275b743da..ee605d06cc2c51 100644 --- a/src/staticdata.c +++ b/src/staticdata.c @@ -88,6 +88,8 @@ External links: #include "valgrind.h" #include "julia_assert.h" +static const size_t WORLD_AGE_REVALIDATION_SENTINEL = 0x1; + #include "staticdata_utils.c" #include "precompile_utils.c" @@ -501,6 +503,8 @@ typedef struct { jl_array_t *link_ids_gvars; jl_array_t *link_ids_external_fnvars; jl_ptls_t ptls; + // Set (implemented has a hasmap of MethodInstances to themselves) of which MethodInstances have (forward) edges + // to other MethodInstances. htable_t callers_with_edges; jl_image_t *image; int8_t incremental; @@ -1596,37 +1600,37 @@ static void jl_write_values(jl_serializer_state *s) JL_GC_DISABLED } else if (jl_is_code_instance(v)) { assert(f == s->s); + // Handle the native-code pointers - assert(f == s->s); - jl_code_instance_t *m = (jl_code_instance_t*)v; - jl_code_instance_t *newm = (jl_code_instance_t*)&f->buf[reloc_offset]; + jl_code_instance_t *ci = (jl_code_instance_t*)v; + jl_code_instance_t *newci = (jl_code_instance_t*)&f->buf[reloc_offset]; if (s->incremental) { arraylist_push(&s->fixup_objs, (void*)reloc_offset); - if (m->min_world > 1) - newm->min_world = ~(size_t)0; // checks that we reprocess this upon deserialization - if (m->max_world != ~(size_t)0) - newm->max_world = 0; + if (ci->min_world > 1) + newci->min_world = ~(size_t)0; // checks that we reprocess this upon deserialization + if (ci->max_world != ~(size_t)0) + newci->max_world = 0; else { - if (jl_atomic_load_relaxed(&m->inferred) && ptrhash_has(&s->callers_with_edges, m->def)) - newm->max_world = 1; // sentinel value indicating this will need validation - if (m->min_world > 0 && jl_atomic_load_relaxed(&m->inferred) ) { + if (jl_atomic_load_relaxed(&ci->inferred) && ptrhash_has(&s->callers_with_edges, ci->def)) + newci->max_world = WORLD_AGE_REVALIDATION_SENTINEL; + if (ci->min_world > 0 && jl_atomic_load_relaxed(&ci->inferred) ) { // TODO: also check if this object is part of the codeinst cache // will check on deserialize if this cache entry is still valid } } } - jl_atomic_store_relaxed(&newm->invoke, NULL); - jl_atomic_store_relaxed(&newm->specsigflags, 0); - jl_atomic_store_relaxed(&newm->specptr.fptr, NULL); + jl_atomic_store_relaxed(&newci->invoke, NULL); + jl_atomic_store_relaxed(&newci->specsigflags, 0); + jl_atomic_store_relaxed(&newci->specptr.fptr, NULL); int8_t fptr_id = JL_API_NULL; int8_t builtin_id = 0; - if (jl_atomic_load_relaxed(&m->invoke) == jl_fptr_const_return) { + if (jl_atomic_load_relaxed(&ci->invoke) == jl_fptr_const_return) { fptr_id = JL_API_CONST; } else { - if (jl_is_method(m->def->def.method)) { - builtin_id = jl_fptr_id(jl_atomic_load_relaxed(&m->specptr.fptr)); + if (jl_is_method(ci->def->def.method)) { + builtin_id = jl_fptr_id(jl_atomic_load_relaxed(&ci->specptr.fptr)); if (builtin_id) { // found in the table of builtins assert(builtin_id >= 2); fptr_id = JL_API_BUILTIN; @@ -1634,7 +1638,7 @@ static void jl_write_values(jl_serializer_state *s) JL_GC_DISABLED else { int32_t invokeptr_id = 0; int32_t specfptr_id = 0; - jl_get_function_id(native_functions, m, &invokeptr_id, &specfptr_id); // see if we generated code for it + jl_get_function_id(native_functions, ci, &invokeptr_id, &specfptr_id); // see if we generated code for it if (invokeptr_id) { if (invokeptr_id == -1) { fptr_id = JL_API_BOXED; @@ -1666,7 +1670,7 @@ static void jl_write_values(jl_serializer_state *s) JL_GC_DISABLED } } } - jl_atomic_store_relaxed(&newm->invoke, NULL); // relocation offset + jl_atomic_store_relaxed(&newci->invoke, NULL); // relocation offset if (fptr_id != JL_API_NULL) { assert(fptr_id < BuiltinFunctionTag && "too many functions to serialize"); arraylist_push(&s->relocs_list, (void*)(reloc_offset + offsetof(jl_code_instance_t, invoke))); // relocation location @@ -2514,7 +2518,7 @@ JL_DLLEXPORT jl_value_t *jl_as_global_root(jl_value_t *val, int insert) } static void jl_prepare_serialization_data(jl_array_t *mod_array, jl_array_t *newly_inferred, uint64_t worklist_key, - /* outputs */ jl_array_t **extext_methods, jl_array_t **new_specializations, + /* outputs */ jl_array_t **extext_methods, jl_array_t **new_ext_cis, jl_array_t **method_roots_list, jl_array_t **ext_targets, jl_array_t **edges) { // extext_methods: [method1, ...], worklist-owned "extending external" methods added to functions owned by modules outside the worklist @@ -2526,7 +2530,7 @@ static void jl_prepare_serialization_data(jl_array_t *mod_array, jl_array_t *new assert(edges_map == NULL); // Save the inferred code from newly inferred, external methods - *new_specializations = queue_external_cis(newly_inferred); + *new_ext_cis = queue_external_cis(newly_inferred); // Collect method extensions and edges data JL_GC_PUSH1(&edges_map); @@ -2552,9 +2556,9 @@ static void jl_prepare_serialization_data(jl_array_t *mod_array, jl_array_t *new *ext_targets = jl_alloc_vec_any(0); *edges = jl_alloc_vec_any(0); *method_roots_list = jl_alloc_vec_any(0); - // Collect the new method roots - jl_collect_new_roots(*method_roots_list, *new_specializations, worklist_key); - jl_collect_edges(*edges, *ext_targets, *new_specializations, world); + // Collect the new method roots for external specializations + jl_collect_new_roots(*method_roots_list, *new_ext_cis, worklist_key); + jl_collect_edges(*edges, *ext_targets, *new_ext_cis, world); } assert(edges_map == NULL); // jl_collect_edges clears this when done @@ -2564,7 +2568,7 @@ static void jl_prepare_serialization_data(jl_array_t *mod_array, jl_array_t *new // In addition to the system image (where `worklist = NULL`), this can also save incremental images with external linkage static void jl_save_system_image_to_stream(ios_t *f, jl_array_t *mod_array, jl_array_t *worklist, jl_array_t *extext_methods, - jl_array_t *new_specializations, jl_array_t *method_roots_list, + jl_array_t *new_ext_cis, jl_array_t *method_roots_list, jl_array_t *ext_targets, jl_array_t *edges) JL_GC_DISABLED { htable_new(&field_replace, 0); @@ -2680,7 +2684,7 @@ static void jl_save_system_image_to_stream(ios_t *f, jl_array_t *mod_array, // Queue method extensions jl_queue_for_serialization(&s, extext_methods); // Queue the new specializations - jl_queue_for_serialization(&s, new_specializations); + jl_queue_for_serialization(&s, new_ext_cis); // Queue the new roots jl_queue_for_serialization(&s, method_roots_list); // Queue the edges @@ -2836,7 +2840,7 @@ static void jl_save_system_image_to_stream(ios_t *f, jl_array_t *mod_array, } jl_write_value(&s, jl_module_init_order); jl_write_value(&s, extext_methods); - jl_write_value(&s, new_specializations); + jl_write_value(&s, new_ext_cis); jl_write_value(&s, method_roots_list); jl_write_value(&s, ext_targets); jl_write_value(&s, edges); @@ -2924,24 +2928,24 @@ JL_DLLEXPORT void jl_create_system_image(void **_native_data, jl_array_t *workli ff = f; } - jl_array_t *mod_array = NULL, *extext_methods = NULL, *new_specializations = NULL; + jl_array_t *mod_array = NULL, *extext_methods = NULL, *new_ext_cis = NULL; jl_array_t *method_roots_list = NULL, *ext_targets = NULL, *edges = NULL; int64_t checksumpos = 0; int64_t checksumpos_ff = 0; int64_t datastartpos = 0; - JL_GC_PUSH6(&mod_array, &extext_methods, &new_specializations, &method_roots_list, &ext_targets, &edges); + JL_GC_PUSH6(&mod_array, &extext_methods, &new_ext_cis, &method_roots_list, &ext_targets, &edges); if (worklist) { mod_array = jl_get_loaded_modules(); // __toplevel__ modules loaded in this session (from Base.loaded_modules_array) // Generate _native_data` if (_native_data != NULL) { jl_prepare_serialization_data(mod_array, newly_inferred, jl_worklist_key(worklist), - &extext_methods, &new_specializations, NULL, NULL, NULL); + &extext_methods, &new_ext_cis, NULL, NULL, NULL); jl_precompile_toplevel_module = (jl_module_t*)jl_array_ptr_ref(worklist, jl_array_len(worklist)-1); - *_native_data = jl_precompile_worklist(worklist, extext_methods, new_specializations); + *_native_data = jl_precompile_worklist(worklist, extext_methods, new_ext_cis); jl_precompile_toplevel_module = NULL; extext_methods = NULL; - new_specializations = NULL; + new_ext_cis = NULL; } jl_write_header_for_incremental(f, worklist, mod_array, udeps, srctextpos, &checksumpos); if (emit_split) { @@ -2964,7 +2968,7 @@ JL_DLLEXPORT void jl_create_system_image(void **_native_data, jl_array_t *workli ct->reentrant_timing |= 0b1000; if (worklist) { jl_prepare_serialization_data(mod_array, newly_inferred, jl_worklist_key(worklist), - &extext_methods, &new_specializations, &method_roots_list, &ext_targets, &edges); + &extext_methods, &new_ext_cis, &method_roots_list, &ext_targets, &edges); if (!emit_split) { write_int32(f, 0); // No clone_targets write_padding(f, LLT_ALIGN(ios_pos(f), JL_CACHE_BYTE_ALIGNMENT) - ios_pos(f)); @@ -2976,7 +2980,7 @@ JL_DLLEXPORT void jl_create_system_image(void **_native_data, jl_array_t *workli } if (_native_data != NULL) native_functions = *_native_data; - jl_save_system_image_to_stream(ff, mod_array, worklist, extext_methods, new_specializations, method_roots_list, ext_targets, edges); + jl_save_system_image_to_stream(ff, mod_array, worklist, extext_methods, new_ext_cis, method_roots_list, ext_targets, edges); if (_native_data != NULL) native_functions = NULL; // make sure we don't run any Julia code concurrently before this point @@ -3058,7 +3062,7 @@ extern void export_jl_small_typeof(void); static void jl_restore_system_image_from_stream_(ios_t *f, jl_image_t *image, jl_array_t *depmods, uint64_t checksum, /* outputs */ jl_array_t **restored, jl_array_t **init_order, jl_array_t **extext_methods, - jl_array_t **new_specializations, jl_array_t **method_roots_list, + jl_array_t **new_ext_cis, jl_array_t **method_roots_list, jl_array_t **ext_targets, jl_array_t **edges, char **base, arraylist_t *ccallable_list, pkgcachesizes *cachesizes) JL_GC_DISABLED { @@ -3120,7 +3124,7 @@ static void jl_restore_system_image_from_stream_(ios_t *f, jl_image_t *image, jl ios_seek(f, LLT_ALIGN(ios_pos(f), 8)); assert(!ios_eof(f)); s.s = f; - uintptr_t offset_restored = 0, offset_init_order = 0, offset_extext_methods = 0, offset_new_specializations = 0, offset_method_roots_list = 0; + uintptr_t offset_restored = 0, offset_init_order = 0, offset_extext_methods = 0, offset_new_ext_cis = 0, offset_method_roots_list = 0; uintptr_t offset_ext_targets = 0, offset_edges = 0; if (!s.incremental) { size_t i; @@ -3152,7 +3156,7 @@ static void jl_restore_system_image_from_stream_(ios_t *f, jl_image_t *image, jl offset_restored = jl_read_offset(&s); offset_init_order = jl_read_offset(&s); offset_extext_methods = jl_read_offset(&s); - offset_new_specializations = jl_read_offset(&s); + offset_new_ext_cis = jl_read_offset(&s); offset_method_roots_list = jl_read_offset(&s); offset_ext_targets = jl_read_offset(&s); offset_edges = jl_read_offset(&s); @@ -3181,16 +3185,14 @@ static void jl_restore_system_image_from_stream_(ios_t *f, jl_image_t *image, jl uint32_t external_fns_begin = read_uint32(f); jl_read_arraylist(s.s, ccallable_list ? ccallable_list : &s.ccallable_list); if (s.incremental) { - assert(restored && init_order && extext_methods && new_specializations && method_roots_list && ext_targets && edges); + assert(restored && init_order && extext_methods && new_ext_cis && method_roots_list && ext_targets && edges); *restored = (jl_array_t*)jl_delayed_reloc(&s, offset_restored); *init_order = (jl_array_t*)jl_delayed_reloc(&s, offset_init_order); *extext_methods = (jl_array_t*)jl_delayed_reloc(&s, offset_extext_methods); - *new_specializations = (jl_array_t*)jl_delayed_reloc(&s, offset_new_specializations); + *new_ext_cis = (jl_array_t*)jl_delayed_reloc(&s, offset_new_ext_cis); *method_roots_list = (jl_array_t*)jl_delayed_reloc(&s, offset_method_roots_list); *ext_targets = (jl_array_t*)jl_delayed_reloc(&s, offset_ext_targets); *edges = (jl_array_t*)jl_delayed_reloc(&s, offset_edges); - if (!*new_specializations) - *new_specializations = jl_alloc_vec_any(0); } s.s = NULL; @@ -3454,8 +3456,6 @@ static void jl_restore_system_image_from_stream_(ios_t *f, jl_image_t *image, jl jl_code_instance_t *ci = (jl_code_instance_t*)obj; assert(s.incremental); ci->min_world = world; - if (ci->max_world != 0) - jl_array_ptr_1d_push(*new_specializations, (jl_value_t*)ci); } else if (jl_is_globalref(obj)) { continue; // wait until all the module binding tables have been initialized @@ -3601,11 +3601,11 @@ static jl_value_t *jl_restore_package_image_from_stream(void* pkgimage_handle, i assert(datastartpos > 0 && datastartpos < dataendpos); needs_permalloc = jl_options.permalloc_pkgimg || needs_permalloc; jl_value_t *restored = NULL; - jl_array_t *init_order = NULL, *extext_methods = NULL, *new_specializations = NULL, *method_roots_list = NULL, *ext_targets = NULL, *edges = NULL; + jl_array_t *init_order = NULL, *extext_methods = NULL, *new_ext_cis = NULL, *method_roots_list = NULL, *ext_targets = NULL, *edges = NULL; jl_svec_t *cachesizes_sv = NULL; char *base; arraylist_t ccallable_list; - JL_GC_PUSH8(&restored, &init_order, &extext_methods, &new_specializations, &method_roots_list, &ext_targets, &edges, &cachesizes_sv); + JL_GC_PUSH8(&restored, &init_order, &extext_methods, &new_ext_cis, &method_roots_list, &ext_targets, &edges, &cachesizes_sv); { // make a permanent in-memory copy of f (excluding the header) ios_bufmode(f, bm_none); @@ -3629,17 +3629,18 @@ static jl_value_t *jl_restore_package_image_from_stream(void* pkgimage_handle, i ios_close(f); ios_static_buffer(f, sysimg, len); pkgcachesizes cachesizes; - jl_restore_system_image_from_stream_(f, image, depmods, checksum, (jl_array_t**)&restored, &init_order, &extext_methods, &new_specializations, &method_roots_list, &ext_targets, &edges, &base, &ccallable_list, &cachesizes); + jl_restore_system_image_from_stream_(f, image, depmods, checksum, (jl_array_t**)&restored, &init_order, &extext_methods, &new_ext_cis, &method_roots_list, + &ext_targets, &edges, &base, &ccallable_list, &cachesizes); JL_SIGATOMIC_END(); // Insert method extensions jl_insert_methods(extext_methods); - // No special processing of `new_specializations` is required because recaching handled it + // No special processing of `new_ext_cis` is required because recaching handled it // Add roots to methods jl_copy_roots(method_roots_list, jl_worklist_key((jl_array_t*)restored)); // Handle edges size_t world = jl_atomic_load_acquire(&jl_world_counter); - jl_insert_backedges((jl_array_t*)edges, (jl_array_t*)ext_targets, (jl_array_t*)new_specializations, world); // restore external backedges (needs to be last) + jl_insert_backedges((jl_array_t*)edges, (jl_array_t*)ext_targets, (jl_array_t*)new_ext_cis, world); // restore external backedges (needs to be last) // reinit ccallables jl_reinit_ccallable(&ccallable_list, base, pkgimage_handle); arraylist_free(&ccallable_list); @@ -3653,7 +3654,8 @@ static jl_value_t *jl_restore_package_image_from_stream(void* pkgimage_handle, i jl_svecset(cachesizes_sv, 4, jl_box_long(cachesizes.reloclist)); jl_svecset(cachesizes_sv, 5, jl_box_long(cachesizes.gvarlist)); jl_svecset(cachesizes_sv, 6, jl_box_long(cachesizes.fptrlist)); - restored = (jl_value_t*)jl_svec(8, restored, init_order, extext_methods, new_specializations, method_roots_list, + restored = (jl_value_t*)jl_svec(8, restored, init_order, extext_methods, + new_ext_cis ? (jl_value_t*)new_ext_cis : jl_nothing, method_roots_list, ext_targets, edges, cachesizes_sv); } else { diff --git a/src/staticdata_utils.c b/src/staticdata_utils.c index 9fded902df0bdd..982f4b104eb748 100644 --- a/src/staticdata_utils.c +++ b/src/staticdata_utils.c @@ -168,7 +168,7 @@ static int has_backedge_to_worklist(jl_method_instance_t *mi, htable_t *visited, // HT_NOTFOUND: not yet analyzed // HT_NOTFOUND + 1: no link back // HT_NOTFOUND + 2: does link back - // HT_NOTFOUND + 3: does link back, and included in new_specializations already + // HT_NOTFOUND + 3: does link back, and included in new_ext_cis already // HT_NOTFOUND + 4 + depth: in-progress int found = (char*)*bp - (char*)HT_NOTFOUND; if (found) @@ -224,8 +224,8 @@ static jl_array_t *queue_external_cis(jl_array_t *list) size_t n0 = jl_array_nrows(list); htable_new(&visited, n0); arraylist_new(&stack, 0); - jl_array_t *new_specializations = jl_alloc_vec_any(0); - JL_GC_PUSH1(&new_specializations); + jl_array_t *new_ext_cis = jl_alloc_vec_any(0); + JL_GC_PUSH1(&new_ext_cis); for (i = n0; i-- > 0; ) { jl_code_instance_t *ci = (jl_code_instance_t*)jl_array_ptr_ref(list, i); assert(jl_is_code_instance(ci)); @@ -241,7 +241,7 @@ static jl_array_t *queue_external_cis(jl_array_t *list) void **bp = ptrhash_bp(&visited, mi); if (*bp != (void*)((char*)HT_NOTFOUND + 3)) { *bp = (void*)((char*)HT_NOTFOUND + 3); - jl_array_ptr_1d_push(new_specializations, (jl_value_t*)ci); + jl_array_ptr_1d_push(new_ext_cis, (jl_value_t*)ci); } } } @@ -249,25 +249,25 @@ static jl_array_t *queue_external_cis(jl_array_t *list) htable_free(&visited); arraylist_free(&stack); JL_GC_POP(); - // reverse new_specializations - n0 = jl_array_nrows(new_specializations); - jl_value_t **news = jl_array_data(new_specializations, jl_value_t*); + // reverse new_ext_cis + n0 = jl_array_nrows(new_ext_cis); + jl_value_t **news = jl_array_data(new_ext_cis, jl_value_t*); for (i = 0; i < n0; i++) { jl_value_t *temp = news[i]; news[i] = news[n0 - i - 1]; news[n0 - i - 1] = temp; } - return new_specializations; + return new_ext_cis; } // New roots for external methods -static void jl_collect_new_roots(jl_array_t *roots, jl_array_t *new_specializations, uint64_t key) +static void jl_collect_new_roots(jl_array_t *roots, jl_array_t *new_ext_cis, uint64_t key) { htable_t mset; htable_new(&mset, 0); - size_t l = new_specializations ? jl_array_nrows(new_specializations) : 0; + size_t l = new_ext_cis ? jl_array_nrows(new_ext_cis) : 0; for (size_t i = 0; i < l; i++) { - jl_code_instance_t *ci = (jl_code_instance_t*)jl_array_ptr_ref(new_specializations, i); + jl_code_instance_t *ci = (jl_code_instance_t*)jl_array_ptr_ref(new_ext_cis, i); assert(jl_is_code_instance(ci)); jl_method_t *m = ci->def->def.method; assert(jl_is_method(m)); @@ -1103,29 +1103,29 @@ static void jl_verify_graph(jl_array_t *edges, jl_array_t *maxvalids2) // Restore backedges to external targets // `edges` = [caller1, targets_indexes1, ...], the list of worklist-owned methods calling external methods. // `ext_targets` is [invokesig1, callee1, matches1, ...], the global set of non-worklist callees of worklist-owned methods. -static void jl_insert_backedges(jl_array_t *edges, jl_array_t *ext_targets, jl_array_t *ci_list, size_t minworld) +static void jl_insert_backedges(jl_array_t *edges, jl_array_t *ext_targets, jl_array_t *ext_ci_list, size_t minworld) { // determine which CodeInstance objects are still valid in our image jl_array_t *valids = jl_verify_edges(ext_targets, minworld); JL_GC_PUSH1(&valids); valids = jl_verify_methods(edges, valids); // consumes edges valids, initializes methods valids jl_verify_graph(edges, valids); // propagates methods valids for each edge - size_t i, l; + + size_t n_ext_cis = ext_ci_list ? jl_array_nrows(ext_ci_list) : 0; + htable_t cis_pending_validation; + htable_new(&cis_pending_validation, n_ext_cis); // next build a map from external MethodInstances to their CodeInstance for insertion - l = jl_array_nrows(ci_list); - htable_t visited; - htable_new(&visited, l); - for (i = 0; i < l; i++) { - jl_code_instance_t *ci = (jl_code_instance_t*)jl_array_ptr_ref(ci_list, i); + for (size_t i = 0; i < n_ext_cis; i++) { + jl_code_instance_t *ci = (jl_code_instance_t*)jl_array_ptr_ref(ext_ci_list, i); assert(ci->min_world == minworld); - if (ci->max_world == 1) { // sentinel value: has edges to external callables - ptrhash_put(&visited, (void*)ci->def, (void*)ci); + if (ci->max_world == WORLD_AGE_REVALIDATION_SENTINEL) { + ptrhash_put(&cis_pending_validation, (void*)ci->def, (void*)ci); } else { assert(ci->max_world == ~(size_t)0); jl_method_instance_t *caller = ci->def; - if (jl_atomic_load_relaxed(&ci->inferred) && jl_rettype_inferred(caller, minworld, ~(size_t)0) == jl_nothing) { + if (jl_atomic_load_relaxed(&ci->inferred) && jl_rettype_inferred(caller, minworld, ~(size_t)0) == jl_nothing) { jl_mi_cache_insert(caller, ci); } //jl_static_show((jl_stream*)ios_stderr, (jl_value_t*)caller); @@ -1134,8 +1134,8 @@ static void jl_insert_backedges(jl_array_t *edges, jl_array_t *ext_targets, jl_a } // next enable any applicable new codes - l = jl_array_nrows(edges) / 2; - for (i = 0; i < l; i++) { + size_t nedges = jl_array_nrows(edges) / 2; + for (size_t i = 0; i < nedges; i++) { jl_method_instance_t *caller = (jl_method_instance_t*)jl_array_ptr_ref(edges, 2 * i); size_t maxvalid = jl_array_data(valids, size_t)[i]; if (maxvalid == ~(size_t)0) { @@ -1162,21 +1162,35 @@ static void jl_insert_backedges(jl_array_t *edges, jl_array_t *ext_targets, jl_a } } // then enable any methods associated with it - void *ci = ptrhash_get(&visited, (void*)caller); + void *ci = ptrhash_get(&cis_pending_validation, (void*)caller); //assert(ci != HT_NOTFOUND); if (ci != HT_NOTFOUND) { - // have some new external code to use + // Update any external CIs and add them to the cache. assert(jl_is_code_instance(ci)); jl_code_instance_t *codeinst = (jl_code_instance_t*)ci; assert(codeinst->min_world == minworld && jl_atomic_load_relaxed(&codeinst->inferred) ); codeinst->max_world = maxvalid; - if (jl_rettype_inferred(caller, minworld, maxvalid) == jl_nothing) { - jl_mi_cache_insert(caller, codeinst); + + if (jl_rettype_inferred(caller, minworld, maxvalid) != jl_nothing) { + // We already got a code instance for this world age range from somewhere else - we don't need + // this one. + continue; + } + + jl_mi_cache_insert(caller, codeinst); + } else { + // Likely internal. Find the CI already in the cache hierarchy. + for (jl_code_instance_t *codeinst = jl_atomic_load_relaxed(&caller->cache); codeinst; codeinst=jl_atomic_load_relaxed(&codeinst->next)) { + if (codeinst->min_world != minworld) + continue; + if (codeinst->max_world != WORLD_AGE_REVALIDATION_SENTINEL) + continue; + codeinst->max_world = maxvalid; } } } + htable_free(&cis_pending_validation); - htable_free(&visited); JL_GC_POP(); } diff --git a/test/precompile.jl b/test/precompile.jl index d87e86ab4911c9..8a6b81e1aa96ed 100644 --- a/test/precompile.jl +++ b/test/precompile.jl @@ -1798,10 +1798,10 @@ precompile_test_harness("PkgCacheInspector") do load_path sv = ccall(:jl_restore_incremental, Any, (Cstring, Any, Cint, Cstring), cachefile, depmods, true, "PCI") end - modules, init_order, external_methods, new_specializations, new_method_roots, external_targets, edges = sv + modules, init_order, external_methods, new_ext_cis, new_method_roots, external_targets, edges = sv m = only(external_methods) @test m.name == :repl_cmd && m.nargs < 2 - @test any(new_specializations) do ci + @test new_ext_cis === nothing || any(new_ext_cis) do ci mi = ci.def mi.specTypes == Tuple{typeof(Base.repl_cmd), Int, String} end From 314d40f1bf956dd49521be20ad48a449c3da6808 Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Fri, 12 Jan 2024 15:18:39 +0100 Subject: [PATCH 6/8] ir: Don't accidentally consider pending statements from previous block (#52863) When IncremetnalCompact deletes and edge between blocks, it needs to go to the target and delete any reference to the edge from the PhiNodes therein. What happened in #52858, is that we had a situtation like: ``` # Block n-1 %a Expr(...) Expr(...) # <- This node is pending at the start of compaction # Block n %b PhiNode ``` To do the deletion, we use `CompactPeekIterator`, which gets a list of original statements and iterates over all the statements inside the range, including pending ones. However, there is a semantic confusion about whether to include all all statements since the previous statement (%a above), or only those statements that are actually attached to the first instruction (%b above). This of course doesn't really matter usually unless you're at a BasicBlock boundary. For the present case, we really do not want the instructions in the previous basic block - the iteration of PhiNodes terminates if it seems a stmt that cannot be in the phi block, so mistakenly seeing one of those instructions causes it to fail to fix a PhiNode that it should have, causing #52858. We fix #52858 by giving the CompactPeekIterator a flag to only return stmts in the correct basic block. While we're at it, also fix the condition for what kinds of statements are allowed in the phi block, as that was clarified more recently than this code was written. --- base/compiler/ssair/ir.jl | 24 +++++++++++++++++++----- test/compiler/irpasses.jl | 24 ++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/base/compiler/ssair/ir.jl b/base/compiler/ssair/ir.jl index 8029f6c57fb83d..759997feb04daf 100644 --- a/base/compiler/ssair/ir.jl +++ b/base/compiler/ssair/ir.jl @@ -1340,8 +1340,8 @@ function kill_edge!(compact::IncrementalCompact, active_bb::Int, from::Int, to:: else stmts = compact.ir.cfg.blocks[to].stmts for stmt in CompactPeekIterator(compact, first(stmts), last(stmts)) - stmt === nothing && continue - isa(stmt, PhiNode) || break + is_valid_phiblock_stmt(stmt) || break + isa(stmt, PhiNode) || continue i = findfirst(x::Int32->x==from, stmt.edges) if i !== nothing deleteat!(stmt.edges, i) @@ -1684,13 +1684,27 @@ struct CompactPeekIterator compact::IncrementalCompact start_idx::Int end_idx::Int + include_stmts_before_start::Bool end +CompactPeekIterator(compact::IncrementalCompact, start_idx::Int, end_idx::Int) = + CompactPeekIterator(compact, start_idx, end_idx, false) + function CompactPeekIterator(compact::IncrementalCompact, start_idx::Int) return CompactPeekIterator(compact, start_idx, 0) end -entry_at_idx(entry::NewNodeInfo, idx::Int) = entry.attach_after ? entry.pos == idx - 1 : entry.pos == idx +function entry_at_idx(entry::NewNodeInfo, idx::Int, start_idx::Int, include_stmts_before_start::Bool) + if entry.attach_after + if !include_stmts_before_start + entry.pos >= start_idx || return false + end + return entry.pos == idx - 1 + else + return entry.pos == idx + end +end + function iterate(it::CompactPeekIterator, (idx, aidx, bidx)::NTuple{3, Int}=(it.start_idx, it.compact.new_nodes_idx, 1)) if it.end_idx > 0 && idx > it.end_idx return nothing @@ -1702,7 +1716,7 @@ function iterate(it::CompactPeekIterator, (idx, aidx, bidx)::NTuple{3, Int}=(it. if compact.new_nodes_idx <= length(compact.perm) new_nodes = compact.ir.new_nodes for eidx in aidx:length(compact.perm) - if entry_at_idx(new_nodes.info[compact.perm[eidx]], idx) + if entry_at_idx(new_nodes.info[compact.perm[eidx]], idx, it.start_idx, it.include_stmts_before_start) entry = new_nodes.stmts[compact.perm[eidx]] return (entry[:stmt], (idx, eidx+1, bidx)) end @@ -1710,7 +1724,7 @@ function iterate(it::CompactPeekIterator, (idx, aidx, bidx)::NTuple{3, Int}=(it. end if !isempty(compact.pending_perm) for eidx in bidx:length(compact.pending_perm) - if entry_at_idx(compact.pending_nodes.info[compact.pending_perm[eidx]], idx) + if entry_at_idx(compact.pending_nodes.info[compact.pending_perm[eidx]], idx, it.start_idx, it.include_stmts_before_start) entry = compact.pending_nodes.stmts[compact.pending_perm[eidx]] return (entry[:stmt], (idx, aidx, eidx+1)) end diff --git a/test/compiler/irpasses.jl b/test/compiler/irpasses.jl index 43027ff740e435..4f4faf3ae66178 100644 --- a/test/compiler/irpasses.jl +++ b/test/compiler/irpasses.jl @@ -1740,3 +1740,27 @@ end return 0 end @test code_typed(f52703)[1][2] === Int + +# Issue #52858 - compaction gets confused by pending node +let code = Any[ + # Block 1 + GotoIfNot(true, 6), + # Block 2 + Expr(:call, println, 1), + Expr(:call, Base.inferencebarrier, true), + GotoIfNot(SSAValue(3), 6), + # Block 3 + nothing, + # Block 4 + PhiNode(Int32[1, 4, 5], Any[1, 2, 3]), + ReturnNode(SSAValue(6)) +] + ir = make_ircode(code) + Core.Compiler.insert_node!(ir, SSAValue(5), + Core.Compiler.NewInstruction( + Expr(:call, println, 2), Nothing, Int32(1)), + #= attach_after = =# true) + ir = Core.Compiler.compact!(ir, true) + @test Core.Compiler.verify_ir(ir) === nothing + @test count(x->isa(x, GotoIfNot), ir.stmts.stmt) == 1 +end From 270ea64d434363624d269306de286ce6d041347b Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Fri, 12 Jan 2024 15:19:07 +0100 Subject: [PATCH 7/8] sroa: Relax scope assertion (#52866) It's possible for this assertion to be violated if there's dead code in the middle of the function. I think the best thing to do here is just to relax the assertion to allow this particular case. Fixes #52819. --- base/compiler/ssair/passes.jl | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/base/compiler/ssair/passes.jl b/base/compiler/ssair/passes.jl index 62dd4305243ec8..cee54db59f14c9 100644 --- a/base/compiler/ssair/passes.jl +++ b/base/compiler/ssair/passes.jl @@ -1147,7 +1147,18 @@ function (this::IntermediaryCollector)(@nospecialize(pi), @nospecialize(ssa)) end function update_scope_mapping!(scope_mapping, bb, val) - @assert (scope_mapping[bb] in (val, SSAValue(0))) + current_mapping = scope_mapping[bb] + if current_mapping != SSAValue(0) + if val == SSAValue(0) + # Unreachable bbs will have SSAValue(0), but can branch into + # try/catch regions. We could validate with the domtree, but that's + # quite expensive for a debug check, so simply allow this without + # making any changes to mapping. + return + end + @assert current_mapping == val + return + end scope_mapping[bb] = val end From 5b6a94da5af35a4aa91759cac2f8db7669a6ec2a Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Fri, 12 Jan 2024 15:20:44 +0100 Subject: [PATCH 8/8] docs: Clarify that `donotdelete` does not affect unreachable code (#52869) In generally accepted compiler terminology, dead code is all code that if removed does not affect the observable behavior of the program. However, people sometimes use the phrase dead code to mean *unreachable* code, which is a subset of dead code (being dead because it is never semantically executed). If one assumes that definition, the docstring for `donotdelete` may be confusing, as it may in fact be deleted from the code if it is unreachable (existence or non-existence in the IR is not ever semantically observable in Julia, so there could not be such an intrinsic, but of course knowing that requires a deep understanding of Julia semantics). Add an extra note to the docs to clarify this point. --- base/docs/basedocs.jl | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/base/docs/basedocs.jl b/base/docs/basedocs.jl index b2d81f482b6468..bc7d001a06f51c 100644 --- a/base/docs/basedocs.jl +++ b/base/docs/basedocs.jl @@ -3312,7 +3312,7 @@ kw"atomic" This function prevents dead-code elimination (DCE) of itself and any arguments passed to it, but is otherwise the lightest barrier possible. In particular, -it is not a GC safepoint, does model an observable heap effect, does not expand +it is not a GC safepoint, does not model an observable heap effect, does not expand to any code itself and may be re-ordered with respect to other side effects (though the total number of executions may not change). @@ -3330,6 +3330,14 @@ unused and delete the entire benchmark code). `donotdelete(1+1)`, no add instruction needs to be executed at runtime and the code is semantically equivalent to `donotdelete(2).` +!!! note + This intrinsic does not affect the semantics of code that is dead because it is + *unreachable*. For example, the body of the function `f(x) = false && donotdelete(x)` + may be deleted in its entirety. The semantics of this intrinsic only guarantee that + *if* the intrinsic is semantically executed, then there is some program state at + which the value of the arguments of this intrinsic were available (in a register, + in memory, etc.). + # Examples ```julia