Skip to content

Commit

Permalink
lowering: Canonicalize to builtins for global assignment
Browse files Browse the repository at this point in the history
This adjusts lowering to emit `setglobal!` for assignment to globals,
thus making the `=` expr head used only for slots in `CodeInfo` and
entirely absent in `IRCode`. The primary reason for this is just
to reduce the number of special cases that compiler passes have
to reason about. In IRCode, `=` was already essentially equivalent
to `setglobal!`, so there's no good reason not to canonicalize.

Finally, the `=` syntax form for globals already gets recognized
specially to insert `convert` calls to their declared binding
type, so this doesn't impose any additional requirements on
lowering to distinguish local from global assignments. In general,
I'd also like to separate syntax and intermediate forms as much
as possible where their semantics differ, which this accomplises
by just using the builtin.

This change is mostly semantically invisible, except that spliced-in
GlobalRefs now declare their binding because they are indistinguishable
from ordinary assignments at the stage where I inserted the lowering.
If we want to, we can preserve the difference, but it'd be a bit
more annoying for not much benefit, because:
1. The spliced in version was only recently made to work anyway, and
2. The semantics of when exactly bindings are declared is still messy
   on master anyway and will get tweaked shortly in further binding
   partitions work.
  • Loading branch information
Keno committed Dec 3, 2024
1 parent f1b0b01 commit 5de6d3b
Show file tree
Hide file tree
Showing 8 changed files with 23 additions and 63 deletions.
8 changes: 1 addition & 7 deletions Compiler/src/abstractinterpretation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -4020,13 +4020,7 @@ function typeinf_local(interp::AbstractInterpreter, frame::InferenceState, nextr
end
effects === nothing || merge_override_effects!(interp, effects, frame)
if lhs !== nothing && rt !== Bottom
if isa(lhs, SlotNumber)
changes = StateUpdate(lhs, VarState(rt, false))
elseif isa(lhs, GlobalRef)
handle_global_assignment!(interp, frame, currsaw_latestworld, lhs, rt)
else
merge_effects!(interp, frame, EFFECTS_UNKNOWN)
end
changes = StateUpdate(lhs::SlotNumber, VarState(rt, false))
end
end
if !has_curr_ssaflag(frame, IR_FLAG_NOTHROW)
Expand Down
11 changes: 1 addition & 10 deletions Compiler/src/optimize.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1408,16 +1408,7 @@ function statement_cost(ex::Expr, line::Int, src::Union{CodeInfo, IRCode}, sptyp
extyp = line == -1 ? Any : argextype(SSAValue(line), src, sptypes)
return extyp === Union{} ? 0 : UNKNOWN_CALL_COST
elseif head === :(=)
if ex.args[1] isa GlobalRef
cost = UNKNOWN_CALL_COST
else
cost = 0
end
a = ex.args[2]
if a isa Expr
cost = plus_saturate(cost, statement_cost(a, -1, src, sptypes, params))
end
return cost
return statement_cost(ex.args[2], -1, src, sptypes, params)
elseif head === :copyast
return 100
end
Expand Down
12 changes: 0 additions & 12 deletions Compiler/src/ssair/EscapeAnalysis.jl
Original file line number Diff line number Diff line change
Expand Up @@ -642,13 +642,6 @@ function analyze_escapes(ir::IRCode, nargs::Int, 𝕃ₒ::AbstractLattice, get_e
escape_invoke!(astate, pc, stmt.args)
elseif head === :new || head === :splatnew
escape_new!(astate, pc, stmt.args)
elseif head === :(=)
lhs, rhs = stmt.args
if isa(lhs, GlobalRef) # global store
add_escape_change!(astate, rhs, ⊤)
else
unexpected_assignment!(ir, pc)
end
elseif head === :foreigncall
escape_foreigncall!(astate, pc, stmt.args)
elseif head === :throw_undef_if_not # XXX when is this expression inserted ?
Expand Down Expand Up @@ -981,11 +974,6 @@ function escape_unanalyzable_obj!(astate::AnalysisState, @nospecialize(obj), obj
return objinfo
end

@noinline function unexpected_assignment!(ir::IRCode, pc::Int)
@eval Main (ir = $ir; pc = $pc)
error("unexpected assignment found: inspect `Main.pc` and `Main.pc`")
end

is_nothrow(ir::IRCode, pc::Int) = has_flag(ir[SSAValue(pc)], IR_FLAG_NOTHROW)

# NOTE if we don't maintain the alias set that is separated from the lattice state, we can do
Expand Down
10 changes: 2 additions & 8 deletions Compiler/src/ssair/verify.jl
Original file line number Diff line number Diff line change
Expand Up @@ -363,14 +363,8 @@ function verify_ir(ir::IRCode, print::Bool=true,
isforeigncall = false
if isa(stmt, Expr)
if stmt.head === :(=)
if stmt.args[1] isa SSAValue
@verify_error "SSAValue as assignment LHS"
raise_error()
end
if stmt.args[2] isa GlobalRef
# undefined GlobalRef as assignment RHS is OK
continue
end
@verify_error "Assignment should have been removed during SSA conversion"
raise_error()
elseif stmt.head === :isdefined
if length(stmt.args) > 2 || (length(stmt.args) == 2 && !isa(stmt.args[2], Bool))
@verify_error "malformed isdefined"
Expand Down
2 changes: 1 addition & 1 deletion Compiler/test/inline.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2111,7 +2111,7 @@ for run_finalizer_escape_test in (run_finalizer_escape_test1, run_finalizer_esca
global finalizer_escape::Int = 0

let src = code_typed1(run_finalizer_escape_test, Tuple{Bool, Bool})
@test any(x->isexpr(x, :(=)), src.code)
@test any(iscall((src, Core.setglobal!)), src.code)
end

let
Expand Down
24 changes: 5 additions & 19 deletions src/interpreter.c
Original file line number Diff line number Diff line change
Expand Up @@ -569,25 +569,11 @@ static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s, size_t ip,
s->locals[n - 1] = rhs;
}
else {
jl_module_t *modu;
jl_sym_t *sym;
// Plain assignment is allowed to create bindings at
// toplevel and only for the current module
int alloc = toplevel;
if (jl_is_globalref(lhs)) {
modu = jl_globalref_mod(lhs);
sym = jl_globalref_name(lhs);
alloc &= modu == s->module;
}
else {
assert(jl_is_symbol(lhs));
modu = s->module;
sym = (jl_sym_t*)lhs;
}
JL_GC_PUSH1(&rhs);
jl_binding_t *b = jl_get_binding_wr(modu, sym, alloc);
jl_checked_assignment(b, modu, sym, rhs);
JL_GC_POP();
// This is an unmodeled error. Our frontend only generates
// legal `=` expressions, but since GlobalRef used to be legal
// here, give a loud error in case any package is modifying
// internals.
jl_error("Invalid IR: Assignment LHS not a Slot");
}
}
else if (head == jl_leave_sym) {
Expand Down
14 changes: 10 additions & 4 deletions src/julia-syntax.scm
Original file line number Diff line number Diff line change
Expand Up @@ -4607,14 +4607,20 @@ f(x) = yt(x)
(cdr cnd)
(list cnd))))))
tests))
(define (emit-assignment-or-setglobal lhs rhs)
(if (globalref? lhs)
(begin
(emit `(global ,lhs))
(emit `(call (top setglobal!) ,(cadr lhs) (inert ,(caddr lhs)) ,rhs)))
(emit `(= ,lhs ,rhs))))
(define (emit-assignment lhs rhs)
(if rhs
(if (valid-ir-rvalue? lhs rhs)
(emit `(= ,lhs ,rhs))
(emit-assignment-or-setglobal lhs rhs)
(let ((rr (make-ssavalue)))
(emit `(= ,rr ,rhs))
(emit `(= ,lhs ,rr))))
(emit `(= ,lhs (null)))) ; in unreachable code (such as after return), still emit the assignment so that the structure of those uses is preserved
(emit-assignment-or-setglobal lhs rr)))
(emit-assignment-or-setglobal lhs `(null))) ; in unreachable code (such as after return), still emit the assignment so that the structure of those uses is preserved
#f)
;; the interpreter loop. `break-labels` keeps track of the labels to jump to
;; for all currently closing break-blocks.
Expand Down Expand Up @@ -4693,7 +4699,7 @@ f(x) = yt(x)
rhs (make-ssavalue))))
(if (not (eq? rr rhs))
(emit `(= ,rr ,rhs)))
(emit `(= ,lhs ,rr))
(emit-assignment-or-setglobal lhs rr)
(if tail (emit-return tail rr))
rr)
(emit-assignment lhs rhs))))))
Expand Down
5 changes: 3 additions & 2 deletions test/syntax.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3713,7 +3713,7 @@ end
module Foreign54607
# Syntactic, not dynamic
try_to_create_binding1() = (Foreign54607.foo = 2)
# GlobalRef is allowed for same-module assignment
# GlobalRef is allowed for same-module assignment and declares the binding
@eval try_to_create_binding2() = ($(GlobalRef(Foreign54607, :foo2)) = 2)
function global_create_binding()
global bar
Expand All @@ -3728,7 +3728,7 @@ module Foreign54607
end
@test_throws ErrorException (Foreign54607.foo = 1)
@test_throws ErrorException Foreign54607.try_to_create_binding1()
@test_throws ErrorException Foreign54607.try_to_create_binding2()
Foreign54607.try_to_create_binding2()
function assign_in_foreign_module()
(Foreign54607.foo = 1)
nothing
Expand All @@ -3744,6 +3744,7 @@ Foreign54607.global_create_binding()
@test isdefined(Foreign54607, :baz)
@test isdefined(Foreign54607, :compiled_assign)
@test isdefined(Foreign54607, :gr_assign)
@test isdefined(Foreign54607, :foo2)
Foreign54607.bar = 8
@test Foreign54607.bar == 8
begin
Expand Down

0 comments on commit 5de6d3b

Please sign in to comment.