Outline potentially undefined globals during lowering #51970

Keno · 2023-11-01T04:15:16Z

Currently [1] it is illegal [2] in IRCode to have a GlobalRef in value position that could potentially throw. This is because in IRCode, we want to assign flags to every statement and if there are multiple things with effects in a statement, we lose precision in tracking which they apply to. However, we currently do allow this in CodeInfo. Now that we're starting to make more use of flags in inference also, this is becoming annoying (as it did for IRCode), so I would like to do this transformation earlier. This is an attempt to do this during lowering. It is not entirely clear that this is precisely the correct place for it. We could alternatively consider doing it during the global resolve pass in method.c, but that currently does not renumber SSAValues, so doing it during the renumbering inside lowering may be easier.

N.B.: This is against #51853, because this needs some of the inference precision improvements in that PR to avoid regressing the try/catch elision tests (which before that PR, we were incorrectly computing effects for statement-position GlobalRefs).

[1] 39c278b
[2]

julia/base/compiler/ssair/verify.jl

Lines 54 to 58 in 2f63cc9

    
           elseif isa(op, GlobalRef) 
        
               if !isdefined(op.mod, op.name) || !isconst(op.mod, op.name) 
        
                   @verify_error "Unbound GlobalRef not allowed in value position" 
        
                   error("") 
        
               end

JeffBezanson · 2023-11-01T18:28:37Z

src/ast.c

+    jl_ast_context_t *ctx = jl_ast_ctx(fl_ctx);
+    jl_sym_t *var = jl_symbol(symbol_name(fl_ctx, args[0]));
+    jl_binding_t *b = jl_get_module_binding(ctx->module, var, 0);
+    return b != NULL ? fl_ctx->T : fl_ctx->F;


Suggested change

return b != NULL ? fl_ctx->T : fl_ctx->F;

return b != NULL && b->value != NULL ? fl_ctx->T : fl_ctx->F;

JeffBezanson · 2023-11-01T18:36:03Z

src/julia-syntax.scm

@@ -5006,6 +5006,15 @@ f(x) = yt(x)
      (set! code (cons e code))
      (set! i (+ i 1))
      (set! locs (cons current-loc locs)))
+    (define (maybe-outline-outerref e)
+      (if (and (outerref? e) (not (nothrow-julia-global (cadr e))))


I think this logic should go in compile-args instead.

JeffBezanson · 2023-11-01T18:47:58Z

base/reflection.jl

@@ -2081,6 +2081,8 @@ function bodyfunction(basemethod::Method)
                    else
                        return nothing
                    end
+                elseif isa(fsym, Core.SSAValue)
+                    fsym = ast.code[fsym.id]


We might want to add the ability to annotate uses as nothrow, for auto-generated globals we know at lowering time will be defined before they're used.

vtjnash · 2023-11-01T20:32:18Z

src/ast.c

+    (void)tosymbol(fl_ctx, args[0], "nothrow-julia-global");
+    jl_ast_context_t *ctx = jl_ast_ctx(fl_ctx);
+    jl_sym_t *var = jl_symbol(symbol_name(fl_ctx, args[0]));
+    jl_binding_t *b = jl_get_module_binding(ctx->module, var, 0);


jl_binding_t *b = jl_get_binding(ctx->module, var);

you need the real binding, not the stub

Doesn't that cause the binding to resolve? Currently merely lowering a function that mentions a symbol does not resolve that binding in case you want to define it to something else later.

alright, jl_get_binding_if_bound then

vtjnash · 2023-11-01T20:34:03Z

test/compiler/inference.jl

@@ -2998,6 +2998,8 @@ end
 @test ig27907(Int, Int, 1, 0) == 0

 # issue #28279
+# ensure that lowering doesn't move these into statement position, which would require renumbering
+using Base: +, -


This state dependence, where we give very different lowering depending on whether something has been used already in the module, seems very ripe for causing future heisenbugs

I agree, but on the other hand, this is supposed to be a storage optimization without semantic effect (though of course there can be bugs).

True. I guess in the future we may want to optimize cases like this more aggressively pre-inference too. I just worry about cases where people may pre-lower the code, then ship it off to another process to execute which as slightly different globals at the time. I think Jeff had some code where he was planning to do that and Distributed.jl does that for remotecall.

…hi block We don't fully set up the interpreter state when evaluating the phi block, because we expect all statements to be valid in value-position (using the IR definition of value position). However, the interpreter was overapproximating the size of the phi-block, leading to the possibility that the evaluation may throw. Correct that by truncating the phi block to the actual last phi. Will be tested in existing tests after #51970.

Keno · 2023-11-02T06:41:27Z

Looks like the sysimage size increase is about 1%. We could claw some of that back with more precise never-undef analysis, but I'm not sure it's worth it at this stage. If lowering will get rewritten in julia eventually anyway, we might as well do it then, since we have a much better algorithm for this in julia already.

…hi block We don't fully set up the interpreter state when evaluating the phi block, because we expect all statements to be valid in value-position (using the IR definition of value position). However, the interpreter was overapproximating the size of the phi-block, leading to the possibility that the evaluation may throw. Correct that by truncating the phi block to the actual last phi. Will be tested in existing tests after #51970.

Keno · 2023-11-02T09:51:24Z

I think this is largely complete. However, we should merge #51990 and #51991 first and then rebase this.

vtjnash · 2023-11-03T02:09:14Z

If you have a chance, how many megabytes larger is that, just for reference? It seems okay, and especially so that it is worthwhile

Keno · 2023-11-03T02:11:03Z

1.5MB

…hi block (#51990) We don't fully set up the interpreter state when evaluating the phi block, because we expect all statements to be valid in value-position (using the IR definition of value position). However, the interpreter was overapproximating the size of the phi-block, leading to the possibility that the evaluation may throw. Correct that by truncating the phi block to the actual last phi. Will be tested in existing tests after #51970.

oscardssmith · 2023-11-06T14:42:10Z

Is this ready to merge?

JeffBezanson · 2023-11-07T22:23:32Z

src/julia-syntax.scm

      (and (pair? e)
-           (memq (car e) '(quote inert top core globalref outerref
+           (memq (car e) '(quote inert top core
                                 slot static_parameter)))))


Should we remove static_parameter from here since it can also throw?

Hmm, I suppose, but for some reason I always see static parameters getting moved out anyway.

JeffBezanson · 2023-11-07T22:27:00Z

src/julia-syntax.scm

 (define (valid-ir-argument? e)
-  (or (simple-atom? e) (symbol? e)
+  (or (simple-atom? e)
+      (and (outerref? e) (nothrow-julia-global (cadr e)))


nothrow-julia-global should be able to accept a globalref as well.

src/julia-syntax.scm

JeffBezanson · 2023-11-07T22:49:26Z

src/julia-syntax.scm

+                        (cons (if (and
+                                   ;; Even if we have side effects, we know that singly-assigned
+                                   ;; locals cannot be affected them, so we can inline them anyway.
+                                   (or simple? (single-assign-var? aval))


I think this or should also accept simple-atom, quote, inert, top and core? (Everything in valid-ir-argument except globals, slots, and static_parameter.)

Tried it and it shaves ~450k off the sysimage.

Yes, that seems reasonable

In #51852, we are coercing a boxed `Union{@NamedTuple{progress::String}, @NamedTuple{progress::Float64}}` to `@NamedTuple{progress::String}` via convert_julia_type. This results in a jl_cgval_t that has a Vboxed that points to a boxed `@NamedTuple{progress::Float64}` but with a `@NamedTuple{progress::String}` type tag that the up upsilonnode code then tries to unbox into the typed PhiC slot. This ends up treating the Float64 as a pointer and crashing in GC. Avoid this by adding a runtime check that the converted value is actually compatible (we already had this kind of check for the non-boxed cases) and then making the unboxing runtime-conditional on the type of the union matching the type of the phic. Fixes #51852

Currently [1] it is illegal [2] in IRCode to have a GlobalRef in value position that could potentially throw. This is because in IRCode, we want to assign flags to every statement and if there are multiple things with effects in a statement, we lose precision in tracking which they apply to. However, we currently do allow this in `CodeInfo`. Now that we're starting to make more use of flags in inference also, this is becoming annoying (as it did for IRCode), so I would like to do this transformation earlier. This is an attempt to do this during lowering. It is not entirely clear that this is precisely the correct place for it. We could alternatively consider doing it during the global resolve pass in method.c, but that currently does not renumber SSAValues, so doing it during the renumbering inside lowering may be easier. [1] 39c278b [2] https://github.com/JuliaLang/julia/blob/2f63cc99fb134fb4adb7f11ba86a4e2ab5adcd48/base/compiler/ssair/verify.jl#L54-L58

…hi block We don't fully set up the interpreter state when evaluating the phi block, because we expect all statements to be valid in value-position (using the IR definition of value position). However, the interpreter was overapproximating the size of the phi-block, leading to the possibility that the evaluation may throw. Correct that by truncating the phi block to the actual last phi. Will be tested in existing tests after #51970.

In particular, JuliaLang/julia#51970 which adds more nodes in lowered ir.

After #51970, `Expr(:static_parameter, i::Int)` is now consistently outlined during the lowering, so there's no longer a need for `abstract_eval_value_expr` to handle this expression. This update removes the outdated handling, clarifying where we need to handle the expression.

This partially reverts #51970, once again allowing throwing GlobalRefs in value position for `CodeInfo`, (but not `IRCode`). The primary motivation for this reversal is that after binding partition the throwiness of a global is world-age dependent and we do not want to have lowering be world-age dependent, because that would require us to rewrite the lowered code upon invalidation (#56649). With respect to the original motivation for this change (being able to use the statement flag in inference), we retain the property that the statement flags apply only to the optimizer version of the statement (i.e. with the GlobalRefs moved out). The only place in inference where we were not doing this, we can rely on `exct` instead. This does assume that `exct` is precise, but we've done a lot of work on that in the past year, so hopefully we can rely on that now. The revert is partial, because we go in the opposite direction in a few places: For `GotoIfNot` cond and `EnterNode` scope operands, we just forbid any `GlobalRef` entirely. Constants are rare in these, so no need to burden inference with the possibility of handling these. We do however, keep (and add appropriate handling code) for GlobalRef arguments to `ReturnNode` to make sure that trivial code remains one statement.

Keno requested a review from JeffBezanson November 1, 2023 04:15

JeffBezanson reviewed Nov 1, 2023

View reviewed changes

vtjnash reviewed Nov 1, 2023

View reviewed changes

Keno mentioned this pull request Nov 2, 2023

interpreter: Fix stacktrace for global incorrectly assumed to be in p… #51990

Merged

Keno mentioned this pull request Nov 3, 2023

inference: Model type propagation through exceptions #51754

Merged

topolarity force-pushed the kf/51852 branch from 47ed081 to 3886803 Compare November 3, 2023 18:42

Base automatically changed from kf/51852 to master November 5, 2023 17:01

oscardssmith added compiler:lowering Syntax lowering (compiler front end, 2nd stage) bugfix This change fixes an existing bug compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) labels Nov 5, 2023

JeffBezanson reviewed Nov 7, 2023

View reviewed changes

src/julia-syntax.scm Show resolved Hide resolved

JeffBezanson reviewed Nov 7, 2023

View reviewed changes

Keno added 5 commits November 9, 2023 02:52

WIP

7417350

WIP

8f28497

Keno and others added 4 commits November 9, 2023 02:52

WIP

e30f340

Fix various misc checks

14c848d

fix some indenting

97bff51

Fix rebase

688b1e1

Keno force-pushed the kf/outlineglobals branch from ea1b1f7 to 9c40f43 Compare November 9, 2023 04:20

Keno changed the title ~~WIP: Outline potentially undefined globals during lowering~~ Outline potentially undefined globals during lowering Nov 9, 2023

Address review comments

a0c6204

Keno force-pushed the kf/outlineglobals branch from 9c40f43 to a0c6204 Compare November 9, 2023 04:22

Keno added 2 commits November 9, 2023 04:29

Add GC root annotation

7fa80c1

Update doctest

3ec0c87

JeffBezanson approved these changes Nov 10, 2023

View reviewed changes

Merge branch 'master' into kf/outlineglobals

afd701f

Keno merged commit 16e61e2 into master Nov 11, 2023
6 of 7 checks passed

Keno deleted the kf/outlineglobals branch November 11, 2023 00:47

aviatesk added a commit to aviatesk/JET.jl that referenced this pull request Nov 14, 2023

adjust to JuliaLang/julia#51970

3c48bd3

Pangoraw added a commit to Pangoraw/IRTools.jl that referenced this pull request Dec 11, 2023

make inlining tests more robusts to lowering changes

5cdd662

In particular, JuliaLang/julia#51970 which adds more nodes in lowered ir.

aviatesk mentioned this pull request Jan 4, 2024

remove outdated handling of :static_parameter #52732

Merged

oscardssmith mentioned this pull request Nov 20, 2024

const semantics changed to implicitly global in 1.12-DEV #56613

Open

Keno mentioned this pull request Nov 29, 2024

lowering: Revert undefined global outlining #56714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Outline potentially undefined globals during lowering #51970

Outline potentially undefined globals during lowering #51970

Keno commented Nov 1, 2023

JeffBezanson Nov 1, 2023

JeffBezanson Nov 1, 2023

JeffBezanson Nov 1, 2023

vtjnash Nov 1, 2023

Keno Nov 1, 2023

vtjnash Nov 1, 2023

vtjnash Nov 1, 2023

Keno Nov 1, 2023

vtjnash Nov 2, 2023

Keno commented Nov 2, 2023

Keno commented Nov 2, 2023

vtjnash commented Nov 3, 2023

Keno commented Nov 3, 2023

oscardssmith commented Nov 6, 2023

JeffBezanson Nov 7, 2023

Keno Nov 7, 2023

JeffBezanson Nov 7, 2023

JeffBezanson Nov 7, 2023

JeffBezanson Nov 7, 2023

Keno Nov 8, 2023

	elseif isa(op, GlobalRef)
	if !isdefined(op.mod, op.name) \|\| !isconst(op.mod, op.name)
	@verify_error "Unbound GlobalRef not allowed in value position"
	error("")
	end

	return b != NULL ? fl_ctx->T : fl_ctx->F;
	return b != NULL && b->value != NULL ? fl_ctx->T : fl_ctx->F;

Outline potentially undefined globals during lowering #51970

Outline potentially undefined globals during lowering #51970

Conversation

Keno commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keno commented Nov 2, 2023

Keno commented Nov 2, 2023

vtjnash commented Nov 3, 2023

Keno commented Nov 3, 2023

oscardssmith commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment