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

wasm2c: implement the tail-call proposal #2272

Merged
merged 12 commits into from
Oct 24, 2023
Merged

wasm2c: implement the tail-call proposal #2272

merged 12 commits into from
Oct 24, 2023

Conversation

keithw
Copy link
Member

@keithw keithw commented Jul 30, 2023

This PR implements the tail-call proposal in wasm2c. (Will be needed to update the testsuite -- the exception-handling tests now use tail calls.)

The basic strategy is:

  • If a function doesn't make any tail-calls, it's written unchanged.
  • If a function does make a tail-call, it is written twice: once as the "normal" version, and once as the "tailcallee" version.
  • The normal version has the usual signature, and the tailcallee version has the signature void fn(void **instance_ptr, void *tail_call_stack, wasm_rt_tailcallee_t *next)
  • When a "tailcallee" function starts, it unpacks its params from the tail_call_stack argument, and when it returns, it packs its results back into the tail_call_stack
  • If a "tailcallee" function returns via a return_call or return_call_indirect, it sets next to point to the new function and instance_ptr to point to its originating module instance. If it returns normally, it sets next to NULL.
  • When a "normal" function does a return_call or return_call_indirect, it implements a trampoline: it allocates space for the tail_call_stack, calls the tailcallee version of function, and then loops forever until next is NULL.

To simplify the generated code:

  • when tail-calling a function that's known not to make a tail-call itself, the generated code just does a normal call instead. This allows wasm2c to avoid generating a lot of extra "tailcallee" versions of functions that don't need them.
  • when importing a function, a weak import is used to supply "default" tail-call behavior (converting it into a normal call). This allows the host to avoid having to implement a "tailcallee" version of every import (e.g. the spectest imports, WASI, etc.) just in case it gets tail-called.

@keithw keithw force-pushed the w2c-tail-call branch 2 times, most recently from d446ede to e677efa Compare July 30, 2023 17:56
@keithw keithw force-pushed the w2c-function-cleanups branch 3 times, most recently from 2b3e918 to 65f434e Compare July 31, 2023 16:22
Base automatically changed from w2c-function-cleanups to main July 31, 2023 16:41
@keithw keithw force-pushed the w2c-tail-call branch 8 times, most recently from 0fab24a to 94bcf25 Compare August 25, 2023 06:52
@keithw keithw marked this pull request as ready for review August 25, 2023 07:50
@keithw keithw changed the base branch from main to w2c-spills August 25, 2023 07:57
@keithw keithw force-pushed the w2c-tail-call branch 2 times, most recently from 5084240 to 7ca62d2 Compare September 20, 2023 17:53
@shravanrn
Copy link
Collaborator

(fwiw: I tried to write "have you considered __attribute__((musttail)) to directly enforce tail-call" here.. But it seems it is not suitable for the purpose as it is too restrictive llvm/llvm-project#54964 and anyway we still need "tailcalee" variant of the function to support importing.)

Interesting! Thanks for sharing this. @keithw would probably know more, but I'll also try to have a look to see if we can use to ensure that the C code being generated would be tail-called when compiled with gcc/clang

@keithw keithw requested a review from sbc100 October 2, 2023 16:45
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm % comments

wasm2c/wasm-rt.h Outdated Show resolved Hide resolved
wasm2c/wasm-rt.h Outdated Show resolved Hide resolved
#if (__STDC_VERSION__ >= 201112L) || defined(_Static_assert)
#define wasm_static_assert(X) _Static_assert(X, "assertion failure")
#else
#define wasm_static_assert(X) \
Copy link
Member

Choose a reason for hiding this comment

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

Is the wasm_ prefix the correct one to use here? Why not wasm_rt_?

}

/* The C symbol for a tail-callee export from an arbitrary module. */
// static
Copy link
Member

Choose a reason for hiding this comment

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

Looks strange mixing C and C++ comments directly next each other like this.. maybe its fine since the // static is more like the type hint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed that it's a little strange, but it matches the existing style (e.g. on CWriter::ExportName and CWriter::ModuleInstanceTypeName). We could maybe change all these together in a future PR?

src/c-writer.cc Outdated
WriteTailCallFuncDeclaration(
TailCallExportName(import->module_name, import->field_name));
Write(Newline(), "#endif", Newline(), OpenBrace());
Write("next->fn = NULL;", Newline());
Copy link
Member

Choose a reason for hiding this comment

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

Would it be readable/maintainable to define a macro in the internal header rather than try to stamp out the if/else/endif here?

Copy link
Member Author

@keithw keithw Oct 3, 2023

Choose a reason for hiding this comment

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

Done (or at least attempted -- doing the #pragma inside a #define was a little tricky but hopefully the CI passes on Windows).

src/c-writer.cc Show resolved Hide resolved
Copy link
Collaborator

@shravanrn shravanrn left a comment

Choose a reason for hiding this comment

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

(Still need to look at a part of c-writer)

test/wasm2c/hello.txt Outdated Show resolved Hide resolved
src/c-writer.cc Show resolved Hide resolved
test/wasm2c/tail-calls.txt Outdated Show resolved Hide resolved
test/wasm2c/tail-calls.txt Show resolved Hide resolved
test/wasm2c/tail-calls.txt Show resolved Hide resolved
@keithw keithw requested a review from sbc100 October 11, 2023 07:43
Copy link
Collaborator

@shravanrn shravanrn left a comment

Choose a reason for hiding this comment

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

Sorry, one more batch of comments. I finally managed to go through the whole PR this time. Biggest concern is one new source of UB here that we need to avoid.

I think I finally understand the strategy for tailcalls you have chosen and the codegen for this in the example files now makes sense to me. This is nice in that we won't need any C compiler attributes to force a tail-call, since we are doing it by hand in the tail_call_stack.

I can't say I understand 100% of the c-writer code, but it mostly made sense to me. With a little code-dedup and refactoring, I think this is nearly there.

test/pipes.txt Show resolved Hide resolved
test/wasm2c/tail-calls.txt Outdated Show resolved Hide resolved
test/wasm2c/tail-calls.txt Show resolved Hide resolved
src/c-writer.cc Outdated Show resolved Hide resolved
src/c-writer.cc Show resolved Hide resolved
src/c-writer.cc Outdated Show resolved Hide resolved
src/c-writer.cc Outdated Show resolved Hide resolved
src/c-writer.cc Outdated Show resolved Hide resolved
#else
#define WEAK_FUNC_DECL(func, fallback) \
__attribute__((weak)) void func(void** instance_ptr, void* tail_call_stack, \
wasm_rt_tailcallee_t* next)
Copy link
Member

Choose a reason for hiding this comment

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

This version seems to completely ignore fallback.. how does that work?

Copy link
Collaborator

@shravanrn shravanrn Oct 20, 2023

Choose a reason for hiding this comment

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

i think the fallback is just an alternate way to implement weak symbols in MSVC. IIUC, they aren't used/needed in the non MSVC environments as weak symbols are available.

Copy link
Member

@sbc100 sbc100 Oct 20, 2023

Choose a reason for hiding this comment

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

I guess I don't understand how that fallback function is used on MSVC but a weak symbol is used otherwise. Do we also declare the fallback symbol only under MSVC?

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, the "fallback" parameter is only used on MSVC.

On GCC/clang, the "weak import" is implemented by defining a stub version of the import under its real name, and marking it as a weak symbol so it will be overridden by the real import if it exists.

On MSVC, it's implemented by defining the stub version of the import under the "fallback" name, and then telling the linker to use the fallback as a substitute for the real import if the real import doesn't exist.

@keithw
Copy link
Member Author

keithw commented Oct 23, 2023

@sbc100 Did you want to weigh in again on this one? My thinking is that after we get this in, we can finally update the testsuite and possibly cut a new release.

@keithw
Copy link
Member Author

keithw commented Oct 24, 2023

Ok, merging now, and then let's try to get the testsuite updated, a release cut, and #2308 merged (not necessarily in that order).

@keithw keithw merged commit 6e350ee into main Oct 24, 2023
15 checks passed
@keithw keithw deleted the w2c-tail-call branch October 24, 2023 17:28
@keithw keithw mentioned this pull request Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants