Implement TCO into ReturnCallIndirect and ReturnCallRef in JIT - #450
Implement TCO into ReturnCallIndirect and ReturnCallRef in JIT#450makachanm wants to merge 4 commits into
Conversation
|
The current model is quite complex (and specialized) for me, and add more complexity is proposed. I would prefer a cleaner solution. Perhpas this Then the restart mechanism could be here in some way. Maybe through a special exception or a return code. This way anything could restart anything, there would be less code specialization. The question is not simple, but it is worth thinking about it to avoid too many special cases. |
| if (UNLIKELY(result == TailCallResult::Restart)) { | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
TailCallResult::Restart is used only when JIT is enabled (WALRUS_ENABLE_JIT)
So, please wrap this if statement with TailCallResult::Restart macro
| if (UNLIKELY(result == TailCallResult::Restart)) { | ||
| return nullptr; | ||
| } |
| if (UNLIKELY(result == TailCallResult::Restart)) { | ||
| return nullptr; | ||
| } |
|
|
||
| Operand* operand = instr->operands(); | ||
|
|
||
| bool isReturnCall = instr->opcode() == ByteCode::ReturnCallOpcode |
There was a problem hiding this comment.
This isReturnCall name is quite confusing
What about renaming it as isTailCall?
| if (instr->opcode() == ByteCode::ReturnCallOpcode) { | ||
| ReturnCall* returnCall = reinterpret_cast<ReturnCall*>(instr->byteCode()); | ||
| isSelfDirect = context->compiler->module()->function(returnCall->index()) == context->compiler->moduleFunction(); | ||
| } | ||
|
|
||
| if (isSelfDirect) { |
There was a problem hiding this comment.
It seems that these 2 if conditions could be merged,
and I think that merging is much clearer
|
I am currently working on table64 support for callIndirect / returnCallIndirect #453 |
|
This way is more incomplete to cover all TCO situation can happen, but not using a trampoline method. For example
but I don't think these case will become a practical issue. |
| { | ||
| Vector<size_t> paramBuffer; | ||
| paramBuffer.resizeWithUninitializedValues(parameterOffsetCount); | ||
| size_t stackBuffer[64]; |
There was a problem hiding this comment.
Any specific reason for this 64 size of buffer?
IMO it's better to allocate buffer memory using ALLOCA
ALLOCA allocates the necessary memory on the C stack, which is reclaimed automatically.
So, there is no need to allocate a fixed-size buffer (size: 64) or use the heap (malloc)
| if (idx >= table->size()) { | ||
| context->error = ExecutionContext::UndefinedElementError; | ||
| return ExecutionContext::UndefinedElementError; | ||
| } |
There was a problem hiding this comment.
I wonder if these errors occurred in the callbacks are handled correctly
There was a problem hiding this comment.
I checked that route handles error correctly and throws out into surface. maybe the error code is seems to mismatched, but it is actually correct error code that used in these situation. code wants call the non-existing element in table, so it can considered as correct error code.
| if (LIKELY(targetJitFunction != nullptr && targetJitFunction->isCompiled() | ||
| && targetJitFunction->instanceConstData() == context->currentInstanceConstData | ||
| && targetModuleFunction->requiredStackSize() <= context->frameCapacity)) { |
There was a problem hiding this comment.
AFAIK TCO JIT is only applicable when the callee's stack size is smaller than that of caller.
For the other case, what about allocating a new stack frame and switching to it like that of interpreter's TCO?
3 participants
I found ReturnCallIndirect and ReturnCallRef works in fallback mode that makes stack memory consumption is not a consist.
Problem is they not use a fully stack-frame destructive way to jumping in another function to function situation(so call non-self-call) and also it may be problematic in runtime determination of jump address that used in ReturnCallIndirect and ReturnCallRef.
It cannot handled in in-function flow control method, so I rewrite all JIT TCO code into fully trampoline loop.
It can now full ALL pass the test without stack overflow.