bytecodealliance / wasm-micro-runtime

WebAssembly Micro Runtime (WAMR)
Apache License 2.0
4.96k stars 624 forks source link

aot/jit: tail-call implementation looks wrong #2231

Open yamt opened 1 year ago

yamt commented 1 year ago

Note: llvm c api doesn't seem to have an api to use musttail yet.

wenyongh commented 1 year ago
  • return_call_indirect doesn't seem to use a tail call at all.
  • return_call uses llvm tail attribute, which is merely a hint. the recent versions of llvm has the musttail attirubute, which can be used instead.

Note: llvm c api doesn't seem to have an api to use musttail yet.

Thanks for the reminding, we implemented tail-call long time ago, I remembered we tested the spec cases of that proposal for both interpreter and AOT. And yes, we should test again and try to add the testing to the CI.

For return_call_indirect, maybe it is enough to call_indirect first and then return the result: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/compilation/aot_compiler.c#L413-L416

For the musttail attribute, we may set it with llvm c++ API, we set the function attribute disable-tail-call when needed: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/compilation/aot_llvm_extra.cpp#L319

yamt commented 1 year ago
  • return_call_indirect doesn't seem to use a tail call at all.
  • return_call uses llvm tail attribute, which is merely a hint. the recent versions of llvm has the musttail attirubute, which can be used instead.

Note: llvm c api doesn't seem to have an api to use musttail yet.

Thanks for the reminding, we implemented tail-call long time ago, I remembered we tested the spec cases of that proposal for both interpreter and AOT. And yes, we should test again and try to add the testing to the CI.

FYI, recently llvm fixed a codegen bug in wasm tail-call. that would make tail-call a bit more usable. (thus more chances to expose implementation issues i guess.)

For return_call_indirect, maybe it is enough to call_indirect first and then return the result: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/compilation/aot_compiler.c#L413-L416

it isn't enough because it can consume unbounded amount of stack.

For the musttail attribute, we may set it with llvm c++ API, we set the function attribute disable-tail-call when needed: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/compilation/aot_llvm_extra.cpp#L319

sure.

yamt commented 9 months ago

for some archs (eg xtensa) native tail-call is not available. for such platforms, we might need to invent a special calling convention.

yamt commented 4 months ago

musttail can improve the situation a bit because it bails out earlier. (on aot-compilation time.)

besides the aot-generated code, our "call native" implementations (eg. aot_call_indirect .. invokeNative) need to perform tail call as well, i guess.

stack overflow check wrappers need to perform tail-call as well. (right now it only sometimes does.)

for platforms w/o native tailcall, i don't have any good idea to implement it yet. given the number of platforms where tail call doesn't always work, i'm a bit pessimistic: https://github.com/bytecodealliance/wasm-micro-runtime/blob/0d8ffebd3952a52ca1f2a7ee163549202c3890ed/core/iwasm/compilation/aot_llvm.c#L155-L198