[webkit-reviews] review denied: [Bug 176601] Turn recursive tail calls into loops : [Attachment 322545] Commented out the buggy splitting of entry block
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Oct 3 13:24:50 PDT 2017
Keith Miller <keith_miller at apple.com> has denied Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 176601: Turn recursive tail calls into loops
https://bugs.webkit.org/show_bug.cgi?id=176601
Attachment 322545: Commented out the buggy splitting of entry block
https://bugs.webkit.org/attachment.cgi?id=322545&action=review
--- Comment #57 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 322545
--> https://bugs.webkit.org/attachment.cgi?id=322545
Commented out the buggy splitting of entry block
View in context: https://bugs.webkit.org/attachment.cgi?id=322545&action=review
Seems reasonable but there are some things you should clean up.
> Source/JavaScriptCore/ChangeLog:17
> + - m_linkingTargets is a dictionnary from bytecode indices to
BasicBlock*
typo: dictionnary -> dictionary.
> Source/JavaScriptCore/ChangeLog:21
> + terminal that need to be taken off m_unlinkedBlocks.
typo: needs
> Source/JavaScriptCore/ChangeLog:28
> + - The 7 and 8 arguments of handleCall were inlined in its 3
arguments version for readability
> + - The 9 argument version was cleaned up and simplified
I feel like something is wrong when a function has more than 6 arguments... :(
> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1236
> + BasicBlock* blockPtr = block.ptr();
Nit: you could probably drop this line.
> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1273
> + Node* callTarget = get(VirtualRegister(pc[2].u.operand));
> + int argumentCountIncludingThis = pc[3].u.operand;
> + int registerOffset = -pc[4].u.operand;
It would be great if you gave these names in the BytecodeList.json and used
them here.
> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1325
> +bool ByteCodeParser::handleRecursiveTailCall(Node* callTargetNode,
VirtualRegister thisArgument, CallLinkStatus callLinkStatus)
> +{
> + // We currently only do this optimisation in the simple, non-polymorphic
case.
> + if (callLinkStatus.couldTakeSlowPath() || callLinkStatus.size() != 1)
> + return false;
> +
> + auto targetExecutable = callLinkStatus[0].executable();
> + InlineStackEntry* stackEntry = m_inlineStackTop;
> + do {
> + if (targetExecutable != stackEntry->executable())
> + continue;
> +
> + VERBOSE_LOG(" We found a recursive tail, optimizing it to a jump
to block ", *stackEntry->m_callsiteBlockHead, ".\n");
> + // TODO: set the arguments to the right value
> + // TODO: shadow chicken log
> +
> + // We must also add some check that the profiling information was
correct and the target of this call is what we thought
> + emitFunctionChecks(callLinkStatus[0], callTargetNode, thisArgument);
> +
> + VERBOSE_LOG(" Repeating the work of op_enter as we are jumping to
right after it. \n");
> + Node* undefined = addToGraph(JSConstant,
OpInfo(m_constantUndefined));
> + // Initialize all locals to undefined.
> + for (int i = 0; i < stackEntry->m_codeBlock->m_numVars; ++i)
> + setDirect(stackEntry->remapOperand(virtualRegisterForLocal(i)),
undefined, ImmediateSetWithFlush);
> + // TODO: check that ImmediateSetWithFlush is the right pick
here.
> +
> + Node* jumpNode = addToGraph(Jump);
> + jumpNode->targetBlock() = stackEntry->m_callsiteBlockHead;
> + m_currentBlock->didLink();
> + flushForTerminal();
> + return true;
> + } while ((stackEntry = stackEntry->m_caller));
> +
> + // The tail call was not recursive
> + return false;
This function isn't used, can you remove it for now?
> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1344
> + //if (op == TailCall && handleRecursiveTailCall(callTarget,
thisArgument, callLinkStatus))
> + // return Terminal;
Please remove commented code.
> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2052
> + // TODO: we should be able to get rid of this somehow.
Make this a FIXME and link a bugzilla.
> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2080
> + // TODO: we can avoid this indirection in the case of normal
inlined call by passing the continuation block all the way into
inlineStackEntry->m_continuationBlock in inlineCall.
ditto.
> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4252
> + // to be true. TODO: change this.
Ditto.
> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:5276
> + if (m_inlineStackTop->m_returnValue.isValid()) // TODO:
understand what happens if it is not.
Ditto.
> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:5284
> + // TODO: should there be a special case for when we are
returning from the first block? I do not see why, but there was one.
Ditto.
> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6257
> + BasicBlock* callsiteBlockHead, // TODO: remove from constructor, since
it is set in parseBlock, when dealing with op_enter
Ditto.
More information about the webkit-reviews
mailing list