From aa0758594f0244f33fc4c793c344b1ce227a56eb Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Sun, 7 Jul 2024 00:18:34 +0200 Subject: [PATCH] lowering: Don't resolve type bindings earlier than necessary (#54999) This is a follow up to resolve a TODO left in #54773 as part of preparatory work for #54654. Currently, our lowering for type definition contains an early `isdefined` that forces a decision on binding resolution before the assignment of the actual binding. In the current implementation, this doesn't matter much, but with #54654, this would incur a binding invalidation we would like to avoid. To get around this, we extend the (internal) `isdefined` form to take an extra argument specifying whether or not to permit looking at imported bindings. If not, resolving the binding is not required semantically, but for the purposes of type definition (where assigning to an imported binding would error anyway), this is all we need. --- base/compiler/ssair/verify.jl | 9 ++++++++ base/compiler/validation.jl | 2 +- doc/src/devdocs/ast.md | 7 +++++-- src/builtins.c | 2 +- src/codegen.cpp | 20 +++++++++++------- src/interpreter.c | 9 ++++++-- src/julia-syntax.scm | 8 +++---- src/julia.h | 2 +- src/module.c | 6 +++--- test/syntax.jl | 39 +++++++++++++++++++++++++++++++++++ 10 files changed, 82 insertions(+), 22 deletions(-) diff --git a/base/compiler/ssair/verify.jl b/base/compiler/ssair/verify.jl index bfa5ab82143cb..a4286177e93a4 100644 --- a/base/compiler/ssair/verify.jl +++ b/base/compiler/ssair/verify.jl @@ -346,6 +346,15 @@ function verify_ir(ir::IRCode, print::Bool=true, # undefined GlobalRef as assignment RHS is OK continue end + elseif stmt.head === :isdefined + if length(stmt.args) > 2 || (length(stmt.args) == 2 && !isa(stmt.args[2], Bool)) + @verify_error "malformed isdefined" + error("") + end + if stmt.args[1] isa GlobalRef + # undefined GlobalRef is OK in isdefined + continue + end elseif stmt.head === :gc_preserve_end # We allow gc_preserve_end tokens to span across try/catch # blocks, which isn't allowed for regular SSA values, so diff --git a/base/compiler/validation.jl b/base/compiler/validation.jl index 7a2ff7b4c0f97..ba8e86eeb042c 100644 --- a/base/compiler/validation.jl +++ b/base/compiler/validation.jl @@ -24,7 +24,7 @@ const VALID_EXPR_HEADS = IdDict{Symbol,UnitRange{Int}}( :global => 1:1, :foreigncall => 5:typemax(Int), # name, RT, AT, nreq, (cconv, effects), args..., roots... :cfunction => 5:5, - :isdefined => 1:1, + :isdefined => 1:2, :code_coverage_effect => 0:0, :loopinfo => 0:typemax(Int), :gc_preserve_begin => 0:typemax(Int), diff --git a/doc/src/devdocs/ast.md b/doc/src/devdocs/ast.md index 5db5856422c41..50a64ec5813f7 100644 --- a/doc/src/devdocs/ast.md +++ b/doc/src/devdocs/ast.md @@ -426,8 +426,11 @@ These symbols appear in the `head` field of [`Expr`](@ref)s in lowered form. * `isdefined` - `Expr(:isdefined, :x)` returns a Bool indicating whether `x` has - already been defined in the current scope. + `Expr(:isdefined, :x [, allow_import])` returns a Bool indicating whether `x` has + already been defined in the current scope. The optional second argument is a boolean + that specifies whether `x` should be considered defined by an import or if only constants + or globals in the current module count as being defined. If `x` is not a global, the argument + is ignored. * `the_exception` diff --git a/src/builtins.c b/src/builtins.c index 7f75b28a8a50a..8cc1465592068 100644 --- a/src/builtins.c +++ b/src/builtins.c @@ -1278,7 +1278,7 @@ JL_CALLABLE(jl_f_isdefined) order = jl_memory_order_unordered; if (order < jl_memory_order_unordered) jl_atomic_error("isdefined: module binding cannot be accessed non-atomically"); - int bound = jl_boundp(m, s); // seq_cst always + int bound = jl_boundp(m, s, 1); // seq_cst always return bound ? jl_true : jl_false; } jl_datatype_t *vt = (jl_datatype_t*)jl_typeof(args[0]); diff --git a/src/codegen.cpp b/src/codegen.cpp index 3938a10cb74fd..b886d201e118c 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -956,7 +956,7 @@ static const auto jlboundp_func = new JuliaFunction<>{ [](LLVMContext &C) { auto T_pjlvalue = JuliaType::get_pjlvalue_ty(C); return FunctionType::get(getInt32Ty(C), - {T_pjlvalue, T_pjlvalue}, false); + {T_pjlvalue, T_pjlvalue, getInt32Ty(C)}, false); }, nullptr, }; @@ -5545,7 +5545,7 @@ static jl_cgval_t emit_sparam(jl_codectx_t &ctx, size_t i) return mark_julia_type(ctx, sp, true, jl_any_type); } -static jl_cgval_t emit_isdefined(jl_codectx_t &ctx, jl_value_t *sym) +static jl_cgval_t emit_isdefined(jl_codectx_t &ctx, jl_value_t *sym, int allow_import) { Value *isnull = NULL; if (jl_is_slotnumber(sym) || jl_is_argument(sym)) { @@ -5604,8 +5604,8 @@ static jl_cgval_t emit_isdefined(jl_codectx_t &ctx, jl_value_t *sym) modu = ctx.module; name = (jl_sym_t*)sym; } - jl_binding_t *bnd = jl_get_binding(modu, name); - if (bnd) { + jl_binding_t *bnd = allow_import ? jl_get_binding(modu, name) : jl_get_module_binding(modu, name, 0); + if (bnd && jl_atomic_load_relaxed(&bnd->owner) == bnd) { if (jl_atomic_load_acquire(&bnd->value) != NULL && bnd->constp) return mark_julia_const(ctx, jl_true); Value *bp = julia_binding_gv(ctx, bnd); @@ -5619,7 +5619,8 @@ static jl_cgval_t emit_isdefined(jl_codectx_t &ctx, jl_value_t *sym) else { Value *v = ctx.builder.CreateCall(prepare_call(jlboundp_func), { literal_pointer_val(ctx, (jl_value_t*)modu), - literal_pointer_val(ctx, (jl_value_t*)name) + literal_pointer_val(ctx, (jl_value_t*)name), + ConstantInt::get(getInt32Ty(ctx.builder.getContext()), allow_import) }); isnull = ctx.builder.CreateICmpNE(v, ConstantInt::get(getInt32Ty(ctx.builder.getContext()), 0)); } @@ -6304,8 +6305,13 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaidx_ // however, this is a good way to do it because it should *not* be easy // to add new node types. if (head == jl_isdefined_sym) { - assert(nargs == 1); - return emit_isdefined(ctx, args[0]); + assert(nargs == 1 || nargs == 2); + int allow_import = 1; + if (nargs == 2) { + assert(jl_is_bool(args[1])); + allow_import = args[1] == jl_true; + } + return emit_isdefined(ctx, args[0], allow_import); } else if (head == jl_throw_undef_if_not_sym) { assert(nargs == 2); diff --git a/src/interpreter.c b/src/interpreter.c index 472c6312691ba..5b96c485aac0d 100644 --- a/src/interpreter.c +++ b/src/interpreter.c @@ -232,6 +232,11 @@ static jl_value_t *eval_value(jl_value_t *e, interpreter_state *s) else if (head == jl_isdefined_sym) { jl_value_t *sym = args[0]; int defined = 0; + int allow_import = 1; + if (nargs == 2) { + assert(jl_is_bool(args[1]) && "malformed IR"); + allow_import = args[1] == jl_true; + } if (jl_is_slotnumber(sym) || jl_is_argument(sym)) { ssize_t n = jl_slot_number(sym); if (src == NULL || n > jl_source_nslots(src) || n < 1 || s->locals == NULL) @@ -239,10 +244,10 @@ static jl_value_t *eval_value(jl_value_t *e, interpreter_state *s) defined = s->locals[n - 1] != NULL; } else if (jl_is_globalref(sym)) { - defined = jl_boundp(jl_globalref_mod(sym), jl_globalref_name(sym)); + defined = jl_boundp(jl_globalref_mod(sym), jl_globalref_name(sym), allow_import); } else if (jl_is_symbol(sym)) { - defined = jl_boundp(s->module, (jl_sym_t*)sym); + defined = jl_boundp(s->module, (jl_sym_t*)sym, allow_import); } else if (jl_is_expr(sym) && ((jl_expr_t*)sym)->head == jl_static_parameter_sym) { ssize_t n = jl_unbox_long(jl_exprarg(sym, 0)); diff --git a/src/julia-syntax.scm b/src/julia-syntax.scm index 8ce4f8fa48232..5636caa48e6e6 100644 --- a/src/julia-syntax.scm +++ b/src/julia-syntax.scm @@ -983,8 +983,6 @@ (error (string "field name \"" (deparse v) "\" is not a symbol")))) field-names) `(block - ;; This is to prevent the :isdefined below from resolving the binding to an import. - ;; This will be reworked to a different check with world-age partitioned bindings. (global ,name) (scope-block (block @@ -998,7 +996,7 @@ (call (core svec) ,@attrs) ,mut ,min-initialized)) (call (core _setsuper!) ,name ,super) - (if (isdefined (globalref (thismodule) ,name)) + (if (isdefined (globalref (thismodule) ,name) (false)) (block (= ,prev (globalref (thismodule) ,name)) (if (call (core _equiv_typedef) ,prev ,name) @@ -1061,7 +1059,7 @@ (= ,name (call (core _abstracttype) (thismodule) (inert ,name) (call (core svec) ,@params))) (call (core _setsuper!) ,name ,super) (call (core _typebody!) ,name) - (if (&& (isdefined (globalref (thismodule) ,name)) + (if (&& (isdefined (globalref (thismodule) ,name) (false)) (call (core _equiv_typedef) (globalref (thismodule) ,name) ,name)) (null) (const (globalref (thismodule) ,name) ,name)) @@ -1081,7 +1079,7 @@ (= ,name (call (core _primitivetype) (thismodule) (inert ,name) (call (core svec) ,@params) ,n)) (call (core _setsuper!) ,name ,super) (call (core _typebody!) ,name) - (if (&& (isdefined (globalref (thismodule) ,name)) + (if (&& (isdefined (globalref (thismodule) ,name) (false)) (call (core _equiv_typedef) (globalref (thismodule) ,name) ,name)) (null) (const (globalref (thismodule) ,name) ,name)) diff --git a/src/julia.h b/src/julia.h index 0f9d5b08fe504..6a37736e85142 100644 --- a/src/julia.h +++ b/src/julia.h @@ -1937,7 +1937,7 @@ JL_DLLEXPORT jl_value_t *jl_get_binding_type(jl_module_t *m, jl_sym_t *var); // get binding for assignment JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var, int alloc); JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var); -JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var); +JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var, int allow_import); JL_DLLEXPORT int jl_defines_or_exports_p(jl_module_t *m, jl_sym_t *var); JL_DLLEXPORT int jl_binding_resolved_p(jl_module_t *m, jl_sym_t *var); JL_DLLEXPORT int jl_is_const(jl_module_t *m, jl_sym_t *var); diff --git a/src/module.c b/src/module.c index 52dc6df089215..bfe266ee424f5 100644 --- a/src/module.c +++ b/src/module.c @@ -681,10 +681,10 @@ JL_DLLEXPORT void jl_module_public(jl_module_t *from, jl_sym_t *s, int exported) b->exportp |= exported; } -JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var) // unlike most queries here, this is currently seq_cst +JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var, int allow_import) // unlike most queries here, this is currently seq_cst { - jl_binding_t *b = jl_get_binding(m, var); - return b && (jl_atomic_load(&b->value) != NULL); + jl_binding_t *b = allow_import ? jl_get_binding(m, var) : jl_get_module_binding(m, var, 0); + return b && (jl_atomic_load_relaxed(&b->owner) == b) && (jl_atomic_load(&b->value) != NULL); } JL_DLLEXPORT int jl_defines_or_exports_p(jl_module_t *m, jl_sym_t *var) diff --git a/test/syntax.jl b/test/syntax.jl index 2553056fc00b9..0855c643e1423 100644 --- a/test/syntax.jl +++ b/test/syntax.jl @@ -3865,3 +3865,42 @@ module UndefGlobal54954 end using .UndefGlobal54954: theglobal54954 @test Core.get_binding_type(@__MODULE__, :theglobal54954) === Int + +# Extended isdefined +module ExtendedIsDefined + using Test + module Import + export x2, x3 + x2 = 2 + x3 = 3 + x4 = 4 + end + const x1 = 1 + using .Import + import .Import.x4 + @test x2 == 2 # Resolve the binding + @eval begin + @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x1))) + @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x2))) + @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x3))) + @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x4))) + + @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x1), false)) + @test !$(Expr(:isdefined, GlobalRef(@__MODULE__, :x2), false)) + @test !$(Expr(:isdefined, GlobalRef(@__MODULE__, :x3), false)) + @test !$(Expr(:isdefined, GlobalRef(@__MODULE__, :x4), false)) + end + + @eval begin + @Base.Experimental.force_compile + @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x1))) + @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x2))) + @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x3))) + @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x4))) + + @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x1), false)) + @test !$(Expr(:isdefined, GlobalRef(@__MODULE__, :x2), false)) + @test !$(Expr(:isdefined, GlobalRef(@__MODULE__, :x3), false)) + @test !$(Expr(:isdefined, GlobalRef(@__MODULE__, :x4), false)) + end +end