From 83b7dbbfd196772133e463d6987c80fce8c3d17b Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Mon, 8 Nov 2021 16:43:26 -0500 Subject: [PATCH 1/6] atomics: fix replacefield loop We would previously loop back to the wrong point in the code, and cause potentially chaos and malformed IR. --- src/cgutils.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/cgutils.cpp b/src/cgutils.cpp index 14092d53f0866..2c0c23617cc76 100644 --- a/src/cgutils.cpp +++ b/src/cgutils.cpp @@ -3320,7 +3320,12 @@ static jl_cgval_t emit_setfield(jl_codectx_t &ctx, Value *ptindex = ctx.builder.CreateInBoundsGEP(T_int8, emit_bitcast(ctx, maybe_decay_tracked(ctx, addr), T_pint8), ConstantInt::get(T_size, fsz)); if (needlock) emit_lockstate_value(ctx, strct, true); - BasicBlock *BB = ctx.builder.GetInsertBlock(); + BasicBlock *ModifyBB; + if (ismodifyfield) { + ModifyBB = BasicBlock::Create(jl_LLVMContext, "modify_xchg", ctx.f); + ctx.builder.CreateBr(ModifyBB); + ctx.builder.SetInsertPoint(ModifyBB); + } jl_cgval_t oldval = rhs; if (!issetfield) oldval = emit_unionload(ctx, addr, ptindex, jfty, fsz, al, strct.tbaa, true); @@ -3353,7 +3358,7 @@ static jl_cgval_t emit_setfield(jl_codectx_t &ctx, BasicBlock *XchgBB = BasicBlock::Create(jl_LLVMContext, "xchg", ctx.f); DoneBB = BasicBlock::Create(jl_LLVMContext, "done_xchg", ctx.f); Success = emit_f_is(ctx, oldval, cmp); - ctx.builder.CreateCondBr(Success, XchgBB, ismodifyfield ? BB : DoneBB); + ctx.builder.CreateCondBr(Success, XchgBB, ismodifyfield ? ModifyBB : DoneBB); ctx.builder.SetInsertPoint(XchgBB); } Value *tindex = compute_tindex_unboxed(ctx, rhs_union, jfty); From fc565a47953a2fd17184ae78863a95d35d6ac080 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Wed, 3 Nov 2021 15:17:20 -0400 Subject: [PATCH 2/6] improve jl_pointer_egal for more cases --- src/codegen.cpp | 55 ++++++++++++++++++++----------------------------- src/datatype.c | 6 +++++- 2 files changed, 27 insertions(+), 34 deletions(-) diff --git a/src/codegen.cpp b/src/codegen.cpp index 72cc075b02595..ebc4288b55106 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -2176,19 +2176,27 @@ static jl_cgval_t emit_globalref(jl_codectx_t &ctx, jl_module_t *mod, jl_sym_t * static Value *emit_box_compare(jl_codectx_t &ctx, const jl_cgval_t &arg1, const jl_cgval_t &arg2, Value *nullcheck1, Value *nullcheck2) { - if (jl_pointer_egal(arg1.typ) || jl_pointer_egal(arg2.typ)) { - assert((arg1.isboxed || arg1.constant) && (arg2.isboxed || arg2.constant) && - "Expected unboxed cases to be handled earlier"); - Value *varg1 = arg1.constant ? literal_pointer_val(ctx, arg1.constant) : arg1.V; - Value *varg2 = arg2.constant ? literal_pointer_val(ctx, arg2.constant) : arg2.V; - varg1 = maybe_decay_tracked(ctx, varg1); - varg2 = maybe_decay_tracked(ctx, varg2); - if (cast(varg1->getType())->getAddressSpace() != cast(varg2->getType())->getAddressSpace()) { - varg1 = decay_derived(ctx, varg1); - varg2 = decay_derived(ctx, varg2); + // If either sides is boxed or can be trivially boxed, + // we'll prefer to do a pointer check. + // At this point, we know that at least one of the arguments isn't a constant + // so a runtime content check will involve at least one load from the + // pointer (and likely a type check) + // so a pointer comparison should be no worse than that even in imaging mode + // when the constant pointer has to be loaded. + // Note that we ignore nullcheck, since in the case where it may be set, we + // also knew the types of both fields must be the same so there cannot be + // any unboxed values on either side. + if ((!arg1.TIndex && jl_pointer_egal(arg1.typ)) || (!arg2.TIndex && jl_pointer_egal(arg2.typ))) { + // n.b. Vboxed may be incomplete if Tindex is set (missing singletons) + // and Vboxed == isboxed || Tindex + if ((arg1.Vboxed || arg1.constant) && (arg2.Vboxed || arg2.constant)) { + Value *varg1 = arg1.constant ? literal_pointer_val(ctx, arg1.constant) : maybe_bitcast(ctx, arg1.Vboxed, T_pjlvalue); + Value *varg2 = arg2.constant ? literal_pointer_val(ctx, arg2.constant) : maybe_bitcast(ctx, arg2.Vboxed, T_pjlvalue); + return ctx.builder.CreateICmpEQ(decay_derived(ctx, varg1), decay_derived(ctx, varg2)); } - return ctx.builder.CreateICmpEQ(emit_bitcast(ctx, varg1, T_pint8), - emit_bitcast(ctx, varg2, T_pint8)); + return ConstantInt::get(T_int1, 0); // seems probably unreachable? + // (since intersection of rt1 and rt2 is non-empty here, so we should have + // a value in this intersection, but perhaps intersection might have failed) } return emit_nullcheck_guard2(ctx, nullcheck1, nullcheck2, [&] { @@ -2433,27 +2441,8 @@ static Value *emit_f_is(jl_codectx_t &ctx, const jl_cgval_t &arg1, const jl_cgva }); } - // If either sides is boxed or can be trivially boxed, - // we'll prefer to do a pointer check. - // At this point, we know that at least one of the arguments isn't a constant - // so a runtime content check will involve at least one load from the - // pointer (and likely a type check) - // so a pointer comparison should be no worse than that even in imaging mode - // when the constant pointer has to be loaded. - // Note that we ignore nullcheck, since in the case where it may be set, we - // also knew the types of both fields must be the same so there cannot be - // any unboxed values on either side. - if (jl_pointer_egal(rt1) || jl_pointer_egal(rt2)) { - // n.b. Vboxed == isboxed || Tindex - if (!(arg1.Vboxed || arg1.constant) || !(arg2.Vboxed || arg2.constant)) - return ConstantInt::get(T_int1, 0); - Value *varg1 = arg1.constant ? literal_pointer_val(ctx, arg1.constant) : maybe_bitcast(ctx, arg1.Vboxed, T_pjlvalue); - Value *varg2 = arg2.constant ? literal_pointer_val(ctx, arg2.constant) : maybe_bitcast(ctx, arg2.Vboxed, T_pjlvalue); - return ctx.builder.CreateICmpEQ(decay_derived(ctx, varg1), decay_derived(ctx, varg2)); - } - - // TODO: handle the case where arg1.typ != arg2.typ, or when one of these isn't union, - // or when the union can be pointer + // TODO: handle the case where arg1.typ is not exactly arg2.typ, or when + // one of these isn't union, or when the union can be pointer if (arg1.TIndex && arg2.TIndex && jl_egal(arg1.typ, arg2.typ) && jl_is_uniontype(arg1.typ) && is_uniontype_allunboxed(arg1.typ)) return emit_nullcheck_guard2(ctx, nullcheck1, nullcheck2, [&] { diff --git a/src/datatype.c b/src/datatype.c index 58a4ed8747bca..950a411b75996 100644 --- a/src/datatype.c +++ b/src/datatype.c @@ -317,7 +317,7 @@ int jl_pointer_egal(jl_value_t *t) return 1; if (t == (jl_value_t*)jl_bool_type) return 1; - if (jl_is_mutable_datatype(t) && // excludes abstract types + if (jl_is_mutable_datatype(jl_unwrap_unionall(t)) && // excludes abstract types t != (jl_value_t*)jl_string_type && // technically mutable, but compared by contents t != (jl_value_t*)jl_simplevector_type && !jl_is_kind(t)) @@ -340,6 +340,10 @@ int jl_pointer_egal(jl_value_t *t) return 1; } } + if (jl_is_uniontype(t)) { + jl_uniontype_t *u = (jl_uniontype_t*)t; + return jl_pointer_egal(u->a) && jl_pointer_egal(u->b); + } return 0; } From 340982f394c9dbb01620a2aa0d89413bf674c3f4 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Wed, 3 Nov 2021 15:18:08 -0400 Subject: [PATCH 3/6] codegen: fix needloop for replacefield Wrong previously for String and SimpleVector, and also less optimal than was feasible --- src/cgutils.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/cgutils.cpp b/src/cgutils.cpp index 2c0c23617cc76..a1fef5279a6bd 100644 --- a/src/cgutils.cpp +++ b/src/cgutils.cpp @@ -1676,6 +1676,7 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx, needloop = false; } else if (!isboxed) { + assert(jl_is_concrete_type(jltype)); needloop = ((jl_datatype_t*)jltype)->layout->haspadding; Value *SameType = emit_isa(ctx, cmp, jltype, nullptr).first; if (SameType != ConstantInt::getTrue(jl_LLVMContext)) { @@ -1702,12 +1703,14 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx, if (realelty != elty) Compare = ctx.builder.CreateZExt(Compare, elty); } - else if (cmp.isboxed) { + else if (cmp.isboxed || cmp.constant || jl_pointer_egal(jltype)) { Compare = boxed(ctx, cmp); - needloop = !jl_is_mutable_datatype(jltype); + needloop = !jl_pointer_egal(jltype) && !jl_pointer_egal(cmp.typ); + if (needloop && !cmp.isboxed) // try to use the same box in the compare now and later + cmp = mark_julia_type(ctx, Compare, true, cmp.typ); } else { - Compare = V_rnull; + Compare = V_rnull; // TODO: does this need to be an invalid bit pattern? needloop = true; } } From e325497d279df1c9d0d42efe80b9ccabf83e45ff Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Wed, 3 Nov 2021 16:15:20 -0400 Subject: [PATCH 4/6] use more efficient calling convention for calls with few arguments --- src/codegen.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/codegen.cpp b/src/codegen.cpp index ebc4288b55106..6ab179bb54460 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -1777,7 +1777,7 @@ static std::pair uses_specsig(jl_method_instance_t *lam, jl_value_t // not invalid, consider if specialized signature is worthwhile if (prefer_specsig) return std::make_pair(true, false); - if (!deserves_retbox(rettype) && !jl_is_datatype_singleton((jl_datatype_t*)rettype)) + if (!deserves_retbox(rettype) && !jl_is_datatype_singleton((jl_datatype_t*)rettype) && rettype != (jl_value_t*)jl_bool_type) return std::make_pair(true, false); if (jl_is_uniontype(rettype)) { bool allunbox; @@ -1786,6 +1786,8 @@ static std::pair uses_specsig(jl_method_instance_t *lam, jl_value_t if (nbytes > 0) return std::make_pair(true, false); // some elements of the union could be returned unboxed avoiding allocation } + if (jl_nparams(sig) <= 3) // few parameters == more efficient to pass directly + return std::make_pair(true, false); bool allSingleton = true; for (size_t i = 0; i < jl_nparams(sig); i++) { jl_value_t *sigt = jl_tparam(sig, i); From cfb734306facab5ea998f523e8abce8ee0ab13aa Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Fri, 5 Nov 2021 12:49:27 -0400 Subject: [PATCH 5/6] add more info to SIGUSR1 printing This is similar to what the kernel prints for us when sent SIGINFO, and can be sometimes useful for getting the pid of a process, and other status details. --- src/signal-handling.c | 1 - src/signals-unix.c | 17 +++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/signal-handling.c b/src/signal-handling.c index aef682e29382e..3762c838caba5 100644 --- a/src/signal-handling.c +++ b/src/signal-handling.c @@ -243,7 +243,6 @@ void jl_show_sigill(void *_ctx) // what to do on a critical error on a thread void jl_critical_error(int sig, bt_context_t *context, jl_task_t *ct) { - jl_bt_element_t *bt_data = ct ? ct->ptls->bt_data : NULL; size_t *bt_size = ct ? &ct->ptls->bt_size : NULL; size_t i, n = ct ? *bt_size : 0; diff --git a/src/signals-unix.c b/src/signals-unix.c index 670d57475386f..b9ff5daa1f0d4 100644 --- a/src/signals-unix.c +++ b/src/signals-unix.c @@ -790,19 +790,19 @@ static void *signal_listener(void *arg) } jl_set_safe_restore(old_buf); - jl_ptls_t ptls = jl_all_tls_states[i]; + jl_ptls_t ptls2 = jl_all_tls_states[i]; // store threadid but add 1 as 0 is preserved to indicate end of block - bt_data_prof[bt_size_cur++].uintptr = ptls->tid + 1; + bt_data_prof[bt_size_cur++].uintptr = ptls2->tid + 1; // store task id - bt_data_prof[bt_size_cur++].jlvalue = (jl_value_t*)jl_atomic_load_relaxed(&ptls->current_task); + bt_data_prof[bt_size_cur++].jlvalue = (jl_value_t*)jl_atomic_load_relaxed(&ptls2->current_task); // store cpu cycle clock bt_data_prof[bt_size_cur++].uintptr = cycleclock(); // store whether thread is sleeping but add 1 as 0 is preserved to indicate end of block - bt_data_prof[bt_size_cur++].uintptr = jl_atomic_load_relaxed(&ptls->sleep_check_state) + 1; + bt_data_prof[bt_size_cur++].uintptr = jl_atomic_load_relaxed(&ptls2->sleep_check_state) + 1; // Mark the end of this block with two 0's bt_data_prof[bt_size_cur++].uintptr = 0; @@ -834,6 +834,15 @@ static void *signal_listener(void *arg) jl_exit_thread0(128 + sig, bt_data, bt_size); } else { +#ifndef SIGINFO // SIGINFO already prints this automatically + int nrunning = 0; + for (int idx = jl_n_threads; idx-- > 0; ) { + jl_ptls_t ptls2 = jl_all_tls_states[idx]; + nrunning += !jl_atomic_load_relaxed(&ptls2->sleep_check_state); + } + jl_safe_printf("\ncmd: %s %d running %d of %d\n", jl_options.julia_bin ? jl_options.julia_bin : "julia", jl_getpid(), nrunning, jl_n_threads); +#endif + jl_safe_printf("\nsignal (%d): %s\n", sig, strsignal(sig)); size_t i; for (i = 0; i < bt_size; i += jl_bt_entry_size(bt_data + i)) { From 067ac5e03c0b6674966cc7a1b2075499f4e6cdea Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Mon, 8 Nov 2021 17:17:41 -0500 Subject: [PATCH 6/6] FileWatching: fix typo on #42926 (2539a679ab) --- stdlib/FileWatching/src/FileWatching.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stdlib/FileWatching/src/FileWatching.jl b/stdlib/FileWatching/src/FileWatching.jl index 405f6219fc1cf..fd26b62132047 100644 --- a/stdlib/FileWatching/src/FileWatching.jl +++ b/stdlib/FileWatching/src/FileWatching.jl @@ -563,7 +563,7 @@ function wait(fdw::FDWatcher) events = GC.@preserve fdw _wait(fdw.watcher, fdw.mask) isopen(fdw) || throw(EOFError()) if !events.timedout - @lock fdw.notify fdw.events &= ~events.events + @lock fdw.watcher.notify fdw.watcher.events &= ~events.events return events end end