Make Expr(:invoke) target be a CodeInstance, not MethodInstance #54899

vtjnash · 2024-06-23T05:13:24Z

This is a look at what is necessary and what it would look like for codegen to be using CodeInstance directly to get the API for a function call (specifically the rettype), rather than using a lookup call at that point to decide upon the API. It is based around the idea that eventually we would keep track of these anyways to form a graph of the inferred edge data, for use later in validation anyways (instead of attempting to invert the backedges graph in staticdata_utils.c), so we might as well use the same target type for the :invoke call representation also.

~~This does not depend upon #54894, but is much better with it. It did depend upon #54738, though~~ there still remains work to do to teach it how to select a correct alternative CodeInstance after deserializing. Also still to do: codegen also should be looking for equivalent objects, not just using the exact one given, per the definition of equivalency in https://hackmd.io/@vtjnash/codeinstances (and that is what precompile should be doing also).

Depends on ~~#56552~~, ~~#56551~~, ~~#56547~~, ~~#56598~~

vtjnash · 2024-11-13T21:58:03Z

base/compiler/ssair/inlining.jl

@@ -1539,8 +1549,6 @@ function handle_modifyop!_call!(ir::IRCode, idx::Int, stmt::Expr, info::ModifyOp
    edge = info.edges[1]


@aviatesk is this the right API calls to use to extract the CodeInstance for a result? It is now the only place we call compileable_specialization with a MethodInstance instead of a CodeInstance, so it would be nice to get to simplify this callsite too

There isn’t an API to extract a specific edge from CallInfo, we only have add_edges!. As for handle_modify_op!, some checks are performed beforehand to ensure this code works correctly.

In cases where edge === nothing, it’s usually because inference failed on the inner operations of the modify-function, so it might be fine to simply reject those.

okay, I thought other parts were using getresult, so I wasn't sure if that applied here

Together with #54899, this PR is intending to replicate the functionality of #54373, which allowed particular specializations to have a different ABI signature than what would be suggested by the MethodInstance's `specTypes` field. This PR handles that by adding a special `ABIOverwrite` type, which, when placed in the `owner` field of a `CodeInstance` instructs the system to use the given signature instead.

aviatesk · 2024-11-14T12:35:16Z

base/essentials.jl

@@ -1042,6 +1042,7 @@ call obsolete versions of a function `f`.
    Prior to Julia 1.9, this function was not exported, and was called as `Base.invokelatest`.
 """
 function invokelatest(@nospecialize(f), @nospecialize args...; kwargs...)
+    @inline


Any reason for adding this?

Just noticing that codegen was doing dumb stuff here because of the nospecialize. It would generate a call to make a Tuple for the args so that it could call the apply function to unpack those again to call the builtin 🙄

aviatesk · 2024-11-14T13:06:50Z

base/compiler/ssair/inlining.jl

@@ -1539,8 +1549,6 @@ function handle_modifyop!_call!(ir::IRCode, idx::Int, stmt::Expr, info::ModifyOp
    edge = info.edges[1]


There isn’t an API to extract a specific edge from CallInfo, we only have add_edges!. As for handle_modify_op!, some checks are performed beforehand to ensure this code works correctly.

aviatesk · 2024-11-14T13:07:08Z

base/compiler/ssair/inlining.jl

@@ -1539,8 +1549,6 @@ function handle_modifyop!_call!(ir::IRCode, idx::Int, stmt::Expr, info::ModifyOp
    edge = info.edges[1]


In cases where edge === nothing, it’s usually because inference failed on the inner operations of the modify-function, so it might be fine to simply reject those.

aviatesk · 2024-11-14T13:12:28Z

There is this another place where compileable_specialization(::MethodInstance, ...) is used:

julia/Compiler/src/ssair/inlining.jl

Line 869 in b10df4b

return compileable_specialization(mi, Effects(), et, info, state)

I recall that most of these cases were due to inference failures, but simply doing return nothing here caused some regressions, if I remember correctly.

vtjnash · 2024-11-14T21:48:33Z

simply doing return nothing here caused some regressions

I tried that and confirmed: it especially regresses the case where we found there was a get_compileable_sig different from sig, and that it was therefore wasteful to infer the sig, but in that case we still want to call compileable_specialization to find the get_compileable_sig target and use that as the invoke target. I updated the comment here to mention this.

Keno · 2024-11-18T09:28:26Z

src/interpreter.c

-    assert(jl_is_method_instance(meth));
+    jl_value_t *c = args[0];
+    assert(jl_is_code_instance(c) || jl_is_method_instance(c));
+    jl_method_instance_t *meth = jl_is_method_instance(c) ? (jl_method_instance_t*)c : ((jl_code_instance_t*)c)->def;


This should probably just invoke the codeinstance directly.

Perhaps. We still have to support the legacy thing for a bit longer, at least while we solve a few last inference / optimizer bugs around this

vtjnash · 2024-11-18T20:19:46Z

One stacked PR remaining (#56598), but this is already passing tests, so it can be merged once that is done

vtjnash · 2024-11-20T20:42:41Z

This should be ready to go now, since it seems working, it would be nice to merge before it picks up spurious conflicts.

@nanosoldier runbenchmarks(!"scalar", vs=":master")

Some improvements can be done later:

jitlayers/aotcompile/staticdata should be looking for equivalent objects instead of deferring that entirely to the runtime (as a jl_invoke)
conversely, interpreter should be trying to execute the object directly, instead of immediately looking for an equivalent one
non-nothing owner needs special handling implemented (currently it is still illegal to expect to put a different owner there), since codegen may become inconsistent and incorrect

Make :invoke edges to CodeInstance (with rettype and edges) instead of MethodInstance, for more accuracy (except when inference made a mistake). Remove lookup parameter from codegen, since we no longer do any lookup.

Keno · 2024-11-21T05:12:18Z

@vtjnash small conflicts with my worldage PR, so I rebased this for you, just FYI

nanosoldier · 2024-11-21T07:30:34Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

Although these code paths don’t seem to be used very often.

This PR allows generated functions to return a `CodeInstance` containing optimized IR, allowing them to bypass inference and directly adding inferred code into the ordinary course of execution. This is an enabling capability for various external compiler implementations that may want to provide compilation results to the Julia runtime. As a minimal demonstrator of this capability, this adds a Cassette-like `with_new_compiler` higher-order function, which will compile/execute its arguments with the currently loaded `Compiler` package. Unlike `@activate Compiler[:codegen]`, this change is not global and the cache is fully partitioned. This by itself is a very useful feature when developing Compiler code to be able to test the full end-to-end codegen behavior before the changes are capable of fully self-hosting. A key enabler for this was the recent merging of #54899. This PR includes a hacky version of the second TODO left at the end of that PR, just to make everthing work end-to-end. This PR is working end-to-end, but all three parts of it (the CodeInstance return from generated functions, the `with_new_compiler` feature, and the interpreter integration) need some additional cleanup. This PR is mostly intended as a discussion point for what that additional work needs to be.

vtjnash requested a review from Keno June 23, 2024 05:13

vtjnash force-pushed the jn/codeinfo-edges branch 2 times, most recently from af03fd5 to 91be20b Compare June 24, 2024 20:51

vtjnash force-pushed the jn/codeinfo-edges branch 2 times, most recently from ce5726f to d2c2e51 Compare July 16, 2024 21:46

vtjnash force-pushed the jn/codeinfo-edges branch 3 times, most recently from 2f824bf to 33701f1 Compare July 22, 2024 20:40

vtjnash force-pushed the jn/codeinfo-edges branch from 33701f1 to 4525784 Compare September 17, 2024 14:27

aviatesk force-pushed the jn/codeinfo-edges branch 7 times, most recently from a9c4eeb to fe8d042 Compare September 27, 2024 09:49

aviatesk force-pushed the jn/codeinfo-edges branch 2 times, most recently from a5a2db9 to 6f64fd6 Compare October 4, 2024 06:30

aviatesk force-pushed the jn/codeinfo-edges branch 2 times, most recently from 52efd2b to 2ecae72 Compare October 16, 2024 12:15

aviatesk force-pushed the jn/codeinfo-edges branch 7 times, most recently from 140f560 to 23c3250 Compare October 25, 2024 13:49

aviatesk mentioned this pull request Oct 25, 2024

Add edges vector to CodeInstance/CodeInfo to keep backedges as edges #54894

Merged

vtjnash force-pushed the jn/codeinfo-edges branch 2 times, most recently from d33a71e to 809d3fb Compare October 31, 2024 18:26

vtjnash force-pushed the jn/invoke-codeinstance branch from 2317380 to a7c8401 Compare October 31, 2024 19:32

Base automatically changed from jn/codeinfo-edges to master November 1, 2024 10:15

vtjnash force-pushed the jn/invoke-codeinstance branch from 128e4f2 to 16c53a7 Compare November 5, 2024 22:20

vtjnash commented Nov 13, 2024

View reviewed changes

vtjnash force-pushed the jn/invoke-codeinstance branch from 16c53a7 to b10df4b Compare November 13, 2024 21:59

Keno mentioned this pull request Nov 14, 2024

WIP: Add ABIOverwrite type for owner field #56555

Open

aviatesk reviewed Nov 14, 2024

View reviewed changes

vtjnash force-pushed the jn/invoke-codeinstance branch 2 times, most recently from 94762b7 to a7545af Compare November 14, 2024 22:07

Keno reviewed Nov 18, 2024

View reviewed changes

vtjnash force-pushed the jn/invoke-codeinstance branch from a7545af to 692a21b Compare November 18, 2024 17:32

vtjnash force-pushed the jn/invoke-codeinstance branch from 692a21b to f89d58d Compare November 20, 2024 20:29

vtjnash marked this pull request as ready for review November 20, 2024 20:32

use CodeInstance for :invoke instead of MethodInstance, when possible

cf3e3e3

Make :invoke edges to CodeInstance (with rettype and edges) instead of MethodInstance, for more accuracy (except when inference made a mistake). Remove lookup parameter from codegen, since we no longer do any lookup.

Keno force-pushed the jn/invoke-codeinstance branch from f89d58d to cf3e3e3 Compare November 21, 2024 05:11

vtjnash merged commit c31710a into master Nov 21, 2024
5 of 7 checks passed

vtjnash deleted the jn/invoke-codeinstance branch November 21, 2024 14:10

aviatesk added a commit to JuliaDebug/Cthulhu.jl that referenced this pull request Nov 21, 2024

adjust to JuliaLang/julia#54899

53e13da

Although these code paths don’t seem to be used very often.

Keno mentioned this pull request Nov 22, 2024

WIP: Allow generated functions to return a CodeInstance #56650

Closed

maleadt mentioned this pull request Nov 27, 2024

Remove arraylist_t from external native code APIs. #56693

Merged

giordano mentioned this pull request Nov 27, 2024

Segfault in heavily nested function call #56690

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Expr(:invoke) target be a CodeInstance, not MethodInstance #54899

Make Expr(:invoke) target be a CodeInstance, not MethodInstance #54899

vtjnash commented Jun 23, 2024 •

edited

Loading

vtjnash Nov 13, 2024

aviatesk Nov 14, 2024

aviatesk Nov 14, 2024

vtjnash Nov 14, 2024

aviatesk Nov 14, 2024

vtjnash Nov 14, 2024

aviatesk Nov 14, 2024

aviatesk Nov 14, 2024

aviatesk commented Nov 14, 2024

vtjnash commented Nov 14, 2024

Keno Nov 18, 2024

vtjnash Nov 18, 2024

vtjnash commented Nov 18, 2024

vtjnash commented Nov 20, 2024

Keno commented Nov 21, 2024

nanosoldier commented Nov 21, 2024

		@@ -1539,8 +1549,6 @@ function handle_modifyop!_call!(ir::IRCode, idx::Int, stmt::Expr, info::ModifyOp
		edge = info.edges[1]

Make Expr(:invoke) target be a CodeInstance, not MethodInstance #54899

Make Expr(:invoke) target be a CodeInstance, not MethodInstance #54899

Conversation

vtjnash commented Jun 23, 2024 • edited Loading

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

aviatesk commented Nov 14, 2024

vtjnash commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Nov 18, 2024

vtjnash commented Nov 20, 2024

Keno commented Nov 21, 2024

nanosoldier commented Nov 21, 2024

vtjnash commented Jun 23, 2024 •

edited

Loading