Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Use real wasm tail call instruction #510

Merged
merged 7 commits into from
Jan 22, 2021
Merged

Conversation

ospencer
Copy link
Member

@ospencer ospencer commented Jan 16, 2021

Recreation of #488.

Closes #506.

This is actually a feature instead of a chore because now tail calls work across modules.

@ospencer ospencer self-assigned this Jan 16, 2021
@ospencer ospencer changed the base branch from main to upgrade-binaryenml January 18, 2021 00:13
Base automatically changed from upgrade-binaryenml to main January 18, 2021 00:59
@ospencer ospencer force-pushed the tail-call-instructions branch 2 times, most recently from 90b1f81 to c24e445 Compare January 18, 2021 01:24
@ospencer ospencer marked this pull request as ready for review January 18, 2021 02:00
@ospencer ospencer requested a review from a team January 18, 2021 02:00
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ospencer you are truly a wizard! This is awesome. Just a few questions/comments, but overall looks great.

@@ -589,7 +596,14 @@ let rec compile_comp = (env, c) => {
)
| CLambda(args, body) =>
MAllocate(MClosure(compile_lambda(env, args, body, c.comp_loc)))
| CApp(f, args) =>
| CApp(f, args, true) =>
/* TODO: Utilize MCallKnown */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should actually say use MReturnCallKnown 😛

But it means that we should use return_call when we know what the function we're calling is instead of return_call_indirect. We just don't have that information available in the compiler right now. There's a similar comment for non-tail CApp (which is where I copied this from).

compile_imm(env, f),
List.map(compile_imm(env), args),
)
| CApp(f, args, _) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we match false instead of _ here?

* Further optimizations (such as using only a single function) could be done for directly-recursive functions, but
these are not implemented at this time.
*/;

open Anftree;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this file still needs to exist?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. We still need to tell the compiler when we need to do a tail call.

@@ -922,7 +922,7 @@ let gc = [
"(1, 2)",
),
tgcfile("fib_gc_err", 1024, "fib-gc", "Out of memory"),
tgcfile("fib_gc", 3072, "fib-gc", "832040"),
tgcfile("fib_gc", 4000, "fib-gc", "832040"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean that this goes up to 4000?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just that this doesn't generate the same code anymore (because of the tail call instruction) and that changed the amount of memory this uses. It definitely uses less memory than before, but the way we test this is currently broken (see #504), and the way our testing works makes this number a little arbitrary.

Copy link
Member Author

@ospencer ospencer Jan 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a binary search to find how much memory spanned—that's a better term for what this is—happened and updated it.

@ospencer ospencer merged commit 9c9ffe4 into main Jan 22, 2021
@ospencer ospencer deleted the tail-call-instructions branch January 22, 2021 00:41
@github-actions github-actions bot mentioned this pull request May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler: Invalid tail call optimisation
2 participants