From 0f972a6aed193ed47283abb5e788aa21ef4cefb2 Mon Sep 17 00:00:00 2001 From: Gabriel Baraldi Date: Thu, 6 Jul 2023 14:38:20 -0300 Subject: [PATCH 1/5] Initial test for optimized getfield --- src/codegen.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/codegen.cpp b/src/codegen.cpp index 122170ae3fa97..5a59e65e56e1d 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -3819,6 +3819,18 @@ static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f, return true; } } + else if (fld.typ == (jl_value_t*)jl_symbol_type) { + if (jl_is_datatype(utt) && !jl_is_namedtuple_type(utt)) { // TODO: Look into this for NamedTuple + if (jl_struct_try_layout(utt) && (jl_datatype_nfields(utt) == 1)) { + jl_svec_t *fn = jl_field_names(utt); + assert(jl_svec_len(fn) == 1); + Value *typ_sym = literal_pointer_val(ctx, jl_svecref(fn, 0)); + error_unless(ctx, ctx.builder.CreateICmpEQ(decay_derived(ctx, typ_sym), decay_derived(ctx, fld.V)), "wrong field name "); + *ret = emit_getfield_knownidx(ctx, obj, 0, utt, order); + return true; + } + } + } // TODO: generic getfield func with more efficient calling convention return false; } From 55d2243f33f91bc1e1951118015f62870e038de4 Mon Sep 17 00:00:00 2001 From: Gabriel Baraldi Date: Thu, 6 Jul 2023 16:02:03 -0300 Subject: [PATCH 2/5] Emit correct error Do so by adding a new rtutil function --- src/codegen.cpp | 29 ++++++++++++++++++++++++++++- src/datatype.c | 3 +-- src/jl_exported_funcs.inc | 1 + src/julia.h | 1 + src/rtutils.c | 5 +++++ 5 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/codegen.cpp b/src/codegen.cpp index 5a59e65e56e1d..e22972c9dba57 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -739,6 +739,13 @@ static const auto jlundefvarerror_func = new JuliaFunction<>{ {PointerType::get(JuliaType::get_jlvalue_ty(C), AddressSpace::CalleeRooted)}, false); }, get_attrs_noreturn, }; +static const auto jlhasnofield_func = new JuliaFunction<>{ + XSTR(jl_has_no_field_error), + [](LLVMContext &C) { return FunctionType::get(getVoidTy(C), + {PointerType::get(JuliaType::get_jlvalue_ty(C), AddressSpace::CalleeRooted), + PointerType::get(JuliaType::get_jlvalue_ty(C), AddressSpace::CalleeRooted)}, false); }, + get_attrs_noreturn, +}; static const auto jlboundserrorv_func = new JuliaFunction{ XSTR(jl_bounds_error_ints), [](LLVMContext &C, Type *T_size) { return FunctionType::get(getVoidTy(C), @@ -3318,6 +3325,8 @@ static jl_llvm_functions_t jl_value_t *jlrettype, jl_codegen_params_t ¶ms); +static void emit_hasnofield_error_ifnot(jl_codectx_t &ctx, Value *ok, jl_datatype_t *type, jl_cgval_t name); + static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f, const jl_cgval_t *argv, size_t nargs, jl_value_t *rt, jl_expr_t *ex, bool is_promotable) @@ -3825,7 +3834,8 @@ static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f, jl_svec_t *fn = jl_field_names(utt); assert(jl_svec_len(fn) == 1); Value *typ_sym = literal_pointer_val(ctx, jl_svecref(fn, 0)); - error_unless(ctx, ctx.builder.CreateICmpEQ(decay_derived(ctx, typ_sym), decay_derived(ctx, fld.V)), "wrong field name "); + Value *cond = ctx.builder.CreateICmpEQ(mark_callee_rooted(ctx, typ_sym), mark_callee_rooted(ctx, boxed(ctx, fld))); + emit_hasnofield_error_ifnot(ctx, cond, utt, fld); *ret = emit_getfield_knownidx(ctx, obj, 0, utt, order); return true; } @@ -4624,6 +4634,22 @@ static void undef_var_error_ifnot(jl_codectx_t &ctx, Value *ok, jl_sym_t *name) ctx.builder.SetInsertPoint(ifok); } +static void emit_hasnofield_error_ifnot(jl_codectx_t &ctx, Value *ok, jl_datatype_t *type, jl_cgval_t name) +{ + ++EmittedUndefVarErrors; + assert(name.typ == (jl_value_t*)jl_symbol_type); + BasicBlock *err = BasicBlock::Create(ctx.builder.getContext(), "err", ctx.f); + BasicBlock *ifok = BasicBlock::Create(ctx.builder.getContext(), "ok"); + ctx.builder.CreateCondBr(ok, ifok, err); + ctx.builder.SetInsertPoint(err); + ctx.builder.CreateCall(prepare_call(jlhasnofield_func), + {mark_callee_rooted(ctx, literal_pointer_val(ctx, (jl_value_t*)type)), + mark_callee_rooted(ctx, boxed(ctx, name))}); + ctx.builder.CreateUnreachable(); + ctx.f->getBasicBlockList().push_back(ifok); + ctx.builder.SetInsertPoint(ifok); +} + // returns a jl_ppvalue_t location for the global variable m.s // if the reference currently bound or assign == true, // pbnd will also be assigned with the binding address @@ -9023,6 +9049,7 @@ static void init_jit_functions(void) add_named_global(jlatomicerror_func, &jl_atomic_error); add_named_global(jlthrow_func, &jl_throw); add_named_global(jlundefvarerror_func, &jl_undefined_var_error); + add_named_global(jlhasnofield_func, &jl_has_no_field_error); add_named_global(jlboundserrorv_func, &jl_bounds_error_ints); add_named_global(jlboundserror_func, &jl_bounds_error_int); add_named_global(jlvboundserror_func, &jl_bounds_error_tuple_int); diff --git a/src/datatype.c b/src/datatype.c index 95c3b11c9abdc..d118d4cfa0f91 100644 --- a/src/datatype.c +++ b/src/datatype.c @@ -1558,8 +1558,7 @@ JL_DLLEXPORT int jl_field_index(jl_datatype_t *t, jl_sym_t *fld, int err) } } if (err) - jl_errorf("type %s has no field %s", jl_symbol_name(t->name->name), - jl_symbol_name(fld)); + jl_has_no_field_error(t, fld); return -1; } diff --git a/src/jl_exported_funcs.inc b/src/jl_exported_funcs.inc index 6875e36d6da6a..c2b2a1578fd76 100644 --- a/src/jl_exported_funcs.inc +++ b/src/jl_exported_funcs.inc @@ -510,6 +510,7 @@ XX(jl_uncompress_argname_n) \ XX(jl_uncompress_ir) \ XX(jl_undefined_var_error) \ + XX(jl_has_no_field_error) \ XX(jl_value_ptr) \ XX(jl_ver_is_release) \ XX(jl_ver_major) \ diff --git a/src/julia.h b/src/julia.h index d2eb9a98a4a42..687dbeaa09ab6 100644 --- a/src/julia.h +++ b/src/julia.h @@ -1790,6 +1790,7 @@ JL_DLLEXPORT void JL_NORETURN jl_type_error_rt(const char *fname, jl_value_t *ty JL_MAYBE_UNROOTED, jl_value_t *got JL_MAYBE_UNROOTED); JL_DLLEXPORT void JL_NORETURN jl_undefined_var_error(jl_sym_t *var); +JL_DLLEXPORT void JL_NORETURN jl_has_no_field_error(jl_datatype_t *datatype, jl_sym_t *var); JL_DLLEXPORT void JL_NORETURN jl_atomic_error(char *str); JL_DLLEXPORT void JL_NORETURN jl_bounds_error(jl_value_t *v JL_MAYBE_UNROOTED, jl_value_t *t JL_MAYBE_UNROOTED); diff --git a/src/rtutils.c b/src/rtutils.c index 01ea11014a6db..003c36a3d4230 100644 --- a/src/rtutils.c +++ b/src/rtutils.c @@ -134,6 +134,11 @@ JL_DLLEXPORT void JL_NORETURN jl_undefined_var_error(jl_sym_t *var) jl_throw(jl_new_struct(jl_undefvarerror_type, var)); } +JL_DLLEXPORT void JL_NORETURN jl_has_no_field_error(jl_datatype_t *datatype, jl_sym_t *var) +{ + jl_errorf("type %s has no field %s", jl_symbol_name(datatype->name->name), jl_symbol_name(var)); +} + JL_DLLEXPORT void JL_NORETURN jl_atomic_error(char *str) // == jl_exceptionf(jl_atomicerror_type, "%s", str) { jl_value_t *msg = jl_pchar_to_string((char*)str, strlen(str)); From 5f553d9ba0d2ef4023ad7abd9b0cf447da63fe24 Mon Sep 17 00:00:00 2001 From: Gabriel Baraldi Date: Thu, 6 Jul 2023 16:57:44 -0300 Subject: [PATCH 3/5] Cleanup + test --- src/codegen.cpp | 6 +++--- src/datatype.c | 2 +- src/julia.h | 2 +- src/rtutils.c | 4 ++-- test/compiler/codegen.jl | 9 +++++++++ 5 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/codegen.cpp b/src/codegen.cpp index e22972c9dba57..04f7564dd3e33 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -3325,7 +3325,7 @@ static jl_llvm_functions_t jl_value_t *jlrettype, jl_codegen_params_t ¶ms); -static void emit_hasnofield_error_ifnot(jl_codectx_t &ctx, Value *ok, jl_datatype_t *type, jl_cgval_t name); +static void emit_hasnofield_error_ifnot(jl_codectx_t &ctx, Value *ok, jl_sym_t *type, jl_cgval_t name); static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f, const jl_cgval_t *argv, size_t nargs, jl_value_t *rt, @@ -3835,7 +3835,7 @@ static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f, assert(jl_svec_len(fn) == 1); Value *typ_sym = literal_pointer_val(ctx, jl_svecref(fn, 0)); Value *cond = ctx.builder.CreateICmpEQ(mark_callee_rooted(ctx, typ_sym), mark_callee_rooted(ctx, boxed(ctx, fld))); - emit_hasnofield_error_ifnot(ctx, cond, utt, fld); + emit_hasnofield_error_ifnot(ctx, cond, utt->name->name, fld); *ret = emit_getfield_knownidx(ctx, obj, 0, utt, order); return true; } @@ -4634,7 +4634,7 @@ static void undef_var_error_ifnot(jl_codectx_t &ctx, Value *ok, jl_sym_t *name) ctx.builder.SetInsertPoint(ifok); } -static void emit_hasnofield_error_ifnot(jl_codectx_t &ctx, Value *ok, jl_datatype_t *type, jl_cgval_t name) +static void emit_hasnofield_error_ifnot(jl_codectx_t &ctx, Value *ok, jl_sym_t *type, jl_cgval_t name) { ++EmittedUndefVarErrors; assert(name.typ == (jl_value_t*)jl_symbol_type); diff --git a/src/datatype.c b/src/datatype.c index d118d4cfa0f91..905959fb80e0a 100644 --- a/src/datatype.c +++ b/src/datatype.c @@ -1558,7 +1558,7 @@ JL_DLLEXPORT int jl_field_index(jl_datatype_t *t, jl_sym_t *fld, int err) } } if (err) - jl_has_no_field_error(t, fld); + jl_has_no_field_error(t->name->name, fld); return -1; } diff --git a/src/julia.h b/src/julia.h index 687dbeaa09ab6..5af8a5bc1a170 100644 --- a/src/julia.h +++ b/src/julia.h @@ -1790,7 +1790,7 @@ JL_DLLEXPORT void JL_NORETURN jl_type_error_rt(const char *fname, jl_value_t *ty JL_MAYBE_UNROOTED, jl_value_t *got JL_MAYBE_UNROOTED); JL_DLLEXPORT void JL_NORETURN jl_undefined_var_error(jl_sym_t *var); -JL_DLLEXPORT void JL_NORETURN jl_has_no_field_error(jl_datatype_t *datatype, jl_sym_t *var); +JL_DLLEXPORT void JL_NORETURN jl_has_no_field_error(jl_sym_t *type_name, jl_sym_t *var); JL_DLLEXPORT void JL_NORETURN jl_atomic_error(char *str); JL_DLLEXPORT void JL_NORETURN jl_bounds_error(jl_value_t *v JL_MAYBE_UNROOTED, jl_value_t *t JL_MAYBE_UNROOTED); diff --git a/src/rtutils.c b/src/rtutils.c index 003c36a3d4230..eefd1b25f9bc4 100644 --- a/src/rtutils.c +++ b/src/rtutils.c @@ -134,9 +134,9 @@ JL_DLLEXPORT void JL_NORETURN jl_undefined_var_error(jl_sym_t *var) jl_throw(jl_new_struct(jl_undefvarerror_type, var)); } -JL_DLLEXPORT void JL_NORETURN jl_has_no_field_error(jl_datatype_t *datatype, jl_sym_t *var) +JL_DLLEXPORT void JL_NORETURN jl_has_no_field_error(jl_sym_t *type_name, jl_sym_t *var) { - jl_errorf("type %s has no field %s", jl_symbol_name(datatype->name->name), jl_symbol_name(var)); + jl_errorf("type %s has no field %s", jl_symbol_name(type_name), jl_symbol_name(var)); } JL_DLLEXPORT void JL_NORETURN jl_atomic_error(char *str) // == jl_exceptionf(jl_atomicerror_type, "%s", str) diff --git a/test/compiler/codegen.jl b/test/compiler/codegen.jl index e93ecd232498f..b1520b924b15b 100644 --- a/test/compiler/codegen.jl +++ b/test/compiler/codegen.jl @@ -820,3 +820,12 @@ end # issue 48917, hoisting load to above the parent f48917(x, w) = (y = (a=1, b=x); z = (; a=(a=(1, w), b=(3, y)))) @test f48917(1,2) == (a = (a = (1, 2), b = (3, (a = 1, b = 1))),) + +# https://github.com/JuliaLang/julia/issues/50317 getproperty allocation on struct with 1 field +@testset "getproperty noalloc" + struct Wrapper50317 + lock::ReentrantLock + end + const MONITOR50317 = Wrapper50317(ReentrantLock()) + @test (@allocated getproperty(MONITOR50317,:lock)) == 0 +end \ No newline at end of file From 232238f0773ac5d0ad4514f9c689df81508b30be Mon Sep 17 00:00:00 2001 From: Gabriel Baraldi Date: Thu, 6 Jul 2023 17:31:59 -0300 Subject: [PATCH 4/5] Update test/compiler/codegen.jl Co-authored-by: Jameson Nash --- test/compiler/codegen.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/compiler/codegen.jl b/test/compiler/codegen.jl index b1520b924b15b..1b5d065b887f6 100644 --- a/test/compiler/codegen.jl +++ b/test/compiler/codegen.jl @@ -828,4 +828,4 @@ f48917(x, w) = (y = (a=1, b=x); z = (; a=(a=(1, w), b=(3, y)))) end const MONITOR50317 = Wrapper50317(ReentrantLock()) @test (@allocated getproperty(MONITOR50317,:lock)) == 0 -end \ No newline at end of file +end From 2fe02b25edb581c35793e3c5f2ec892a1d7accc2 Mon Sep 17 00:00:00 2001 From: Gabriel Baraldi Date: Fri, 7 Jul 2023 08:56:43 -0300 Subject: [PATCH 5/5] Update test/compiler/codegen.jl Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> --- test/compiler/codegen.jl | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test/compiler/codegen.jl b/test/compiler/codegen.jl index 1b5d065b887f6..85013ce30d2ca 100644 --- a/test/compiler/codegen.jl +++ b/test/compiler/codegen.jl @@ -822,10 +822,13 @@ f48917(x, w) = (y = (a=1, b=x); z = (; a=(a=(1, w), b=(3, y)))) @test f48917(1,2) == (a = (a = (1, 2), b = (3, (a = 1, b = 1))),) # https://github.com/JuliaLang/julia/issues/50317 getproperty allocation on struct with 1 field -@testset "getproperty noalloc" - struct Wrapper50317 - lock::ReentrantLock - end - const MONITOR50317 = Wrapper50317(ReentrantLock()) - @test (@allocated getproperty(MONITOR50317,:lock)) == 0 +struct Wrapper50317 + lock::ReentrantLock +end +const MONITOR50317 = Wrapper50317(ReentrantLock()) +issue50317() = @noinline MONITOR50317.lock +issue50317() +let res = @timed issue50317() + @test res.bytes == 0 + return res # must return otherwise the compiler may eliminate the result entirely end