New codegen backend: Start emitting code. #971

vext01 · 2024-02-09T15:57:36Z

This makes a start at emitting X86_64 code from the JIT IR.

Obviously this is non-functional at this point (it's currently not even called from the JIT pipeline), but should serve as something we can iterate upon and at least unit test in isolation.

Many things missing:

Trace input handling.
Correct allocation sizes.
Stackmaps
Debugger support.
Loads more testing.
Cross-arch testing support.

This PR is already large, so I've held off doing those for now.

Raising as a draft, as I think this could perhaps use some more tests.

vext01 · 2024-02-09T15:59:28Z

ykrt/src/compile/jitc_yk/codegen/x86_64.rs

+
+    #[test]
+    fn simple_codegen() {
+        let mut aot_mod = aot_ir::Module::default();


@ltratt @ptersilie take a look at this test. Here we have to build an AOT module so that the JIT module can refer to parts of it. It's a bit awkward.

We could make the JIT module independent of the AOT module at the cost of copying. Is it worth the performance hit for testing?

Is it worth the performance hit for testing?

If it's the difference between "testing is intolerable" and "testing is tolerable" then I think "yes".

@ptersilie what do you think?

I dunno. I seems a bit backwards to me to affect your performance for the sake of making testing easier. But if it makes us test less because it's such a hassle then that's also bad. Maybe we could have a general AOT module that we then use for all the tests. Or we make a helper that makes it easier to construct the AOT module. Maybe some syntax where you make a trace and it then generates the corresponding AOT module automatically.

I dunno, since @ltratt was so adamant we never ever copy from the AOT if we can help it, I think he will have to make that decision.

I didn't think about the impact of testing. Given the choice between "make testing plausible" and "squeeze every last drop of performance" I would go for "make testing plausible".

So that's a no on generating the AOT/JIT module automatically from some description for testing?

It sounds like it could be fiddly to be honest.

I think he means that in order to not copy the same type multiple types you need to know if you've already copied it and then return the copy instead.

One could do that, or just use a local hashmap instead. Either way, it's not a big deal IMHO.

We could use a hashmap, yes, but I thought you wouldn't like it. If that's OK then it simplifies some other paces where that idiom is in use too (separate PR).

The other possibility is to have two modes: a testing mode where we copy everything; and a non-testing mode where we reference things. Ultimately "make it testable" is our goal. We won't get everything right, but if that's our goal, at least we'll know what to prioritise.

OK, so I think we should roll will "make it testable". I've made a note for two future PRs based on this discussion. Thanks.

ykrt/src/compile/jitc_yk/codegen/mod.rs

ykrt/src/compile/jitc_yk/codegen/x86_64.rs

ltratt · 2024-02-09T17:49:51Z

ykrt/src/compile/jitc_yk/codegen/x86_64.rs

+
+    #[test]
+    fn simple_codegen() {
+        let mut aot_mod = aot_ir::Module::default();


Is it worth the performance hit for testing?

If it's the difference between "testing is intolerable" and "testing is tolerable" then I think "yes".

ltratt · 2024-02-09T17:51:52Z

This is a decent start! My main suggestion (other than the testing aspect) is that the PR currently uses impl extension in a way that's not very idiomatic and also very constraining. I think we probably want something like:

struct MachineIndependentStuff { ... }

impl MachineIndependentStuff { ... }

trait MachineDependent {
  fn emit_blah(...)
}

and then x86 impls that trait. This is a very rough sketch, though, obviously!

vext01 · 2024-02-12T10:49:00Z

I agree with your concerns about a statically compiled-in backend. I don't like it because it means we can only test one codegen backend on any given arch, when in reality we can test a lot of it, just not execution of another arch.

If it's OK with you, I think that should be another unit of work.

ltratt · 2024-02-12T10:50:17Z

I agree with your concerns about a statically compiled-in backend. I don't like it because it means we can only test one codegen backend on any given arch, when in reality we can test a lot of it, just not execution of another arch.

I really think we need to improve this in this PR.

vext01 · 2024-02-13T12:06:45Z

Note: I've note modularised any of the stuff that might be common between different codegen backends, e.g. the abstract stack and allocation bits. Thinking of kicking that down the line until the time comes for another backend.

ltratt · 2024-02-13T12:07:43Z

Thinking of kicking that down the line until the time comes for another backend.

I suggest that when it's easy to do so we should break things out: and it will often be as easy to break it out as not. But it doesn't have to be a big concern.

vext01 · 2024-02-13T12:19:40Z

I suggest that when it's easy to do so we should break things out

It's easy soonish. We won't know if I got it right until we have another backend of course.

ltratt · 2024-02-13T12:26:50Z

It's easy soonish. We won't know if I got it right until we have another backend of course.

What I meant is: as we add new functionality, let's break it out. We of course won't get it 100% right, but inlining everything (especially when breaking it out is equally easy) is definitely 100% wrong :)

vext01 · 2024-02-13T12:30:16Z

What I meant is: as we add new functionality, let's break it out

So that means break it out right now in this PR. I'll start on that after lunch.

vext01 · 2024-02-19T16:18:17Z

Those last 3 commits do the "splitting out" and kill some unnecessary x86 guards.

If this looks good I can squash and sync the branch.

ltratt · 2024-02-20T19:54:35Z

ykrt/src/compile/jitc_yk/codegen/abs_stack.rs

+/// generation. The abstract stack pointer is zero-based, so the stack pointer value also serves as
+/// the size of the stack.
+///
+/// The implementation is platform agnostic: as the stack gets bigger, the stack pointer grows


s/the stack pointer/abstract stack pointer/

Fixed in 6527226

vext01 · 2024-02-21T08:33:09Z

OK to squash?

ltratt · 2024-02-21T08:35:31Z

Please squash.

vext01 · 2024-02-21T08:42:28Z

splat.

vext01 · 2024-02-21T09:30:50Z

I need to update my local Rust :)

vext01 · 2024-02-21T10:09:13Z

Fixed. OK to squash?

ltratt · 2024-02-21T10:23:11Z

Please squash.

vext01 · 2024-02-21T10:25:42Z

splat.

New codegen backend: Start emitting code.

vext01 · 2024-02-21T11:27:16Z

This failure is leaks being detected by address sanitiser. I guess I have to audit the changes for potential leaks.

ptersilie · 2024-02-21T11:43:36Z

ykrt/src/compile/jitc_yk/jit_ir.rs

@@ -92,6 +99,18 @@ macro_rules! index_16bit {
                self.0.into()
            }
        }
+
+        // impl From<usize> for $struct {


Left over comments?

Fixed in 3eb9bac

vext01 · 2024-02-21T12:16:19Z

As per offline discussion, disable asan for now. See #981

vext01 · 2024-02-21T12:16:39Z

OK to squash?

ltratt · 2024-02-21T12:17:03Z

Please squash.

This makes a start at emitting X86_64 code from the JIT IR. Obviously this is non-functional at this point (it's currently not even called from the JIT pipeline), but should serve as something we can iterate upon and at least unit test in isolation. Many things missing: - Trace input handling. - Correct allocation sizes. - Stackmaps - Debugger support. - Loads more testing. Note we disable asan for now: ykjit#981

vext01 · 2024-02-21T12:22:50Z

splat.

vext01 assigned ltratt Feb 9, 2024

vext01 commented Feb 9, 2024

View reviewed changes

ltratt reviewed Feb 9, 2024

View reviewed changes

vext01 marked this pull request as ready for review February 20, 2024 09:24

ltratt reviewed Feb 20, 2024

View reviewed changes

vext01 force-pushed the gen-code branch from 6527226 to edb2911 Compare February 21, 2024 08:42

ltratt added this pull request to the merge queue Feb 21, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 21, 2024

vext01 force-pushed the gen-code branch from 5b6fd82 to ec2adf9 Compare February 21, 2024 10:25

ltratt added this pull request to the merge queue Feb 21, 2024

github-merge-queue bot pushed a commit that referenced this pull request Feb 21, 2024

Merge pull request #971 from vext01/gen-code

c1d5030

New codegen backend: Start emitting code.

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 21, 2024

ptersilie reviewed Feb 21, 2024

View reviewed changes

vext01 mentioned this pull request Feb 21, 2024

iced_x86 has asan warnings. #981

Closed

vext01 force-pushed the gen-code branch from 789570f to 8d974eb Compare February 21, 2024 12:22

ltratt added this pull request to the merge queue Feb 21, 2024

Merged via the queue into ykjit:master with commit 7c6b99e Feb 21, 2024
2 checks passed

vext01 deleted the gen-code branch February 21, 2024 13:28

New codegen backend: Start emitting code. #971

New codegen backend: Start emitting code. #971

Conversation

vext01 commented Feb 9, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltratt commented Feb 9, 2024

vext01 commented Feb 12, 2024

ltratt commented Feb 12, 2024

vext01 commented Feb 13, 2024

ltratt commented Feb 13, 2024

vext01 commented Feb 13, 2024

ltratt commented Feb 13, 2024

vext01 commented Feb 13, 2024

vext01 commented Feb 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vext01 commented Feb 21, 2024

ltratt commented Feb 21, 2024

vext01 commented Feb 21, 2024

vext01 commented Feb 21, 2024

vext01 commented Feb 21, 2024

ltratt commented Feb 21, 2024

vext01 commented Feb 21, 2024

vext01 commented Feb 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vext01 commented Feb 21, 2024

vext01 commented Feb 21, 2024

ltratt commented Feb 21, 2024

vext01 commented Feb 21, 2024