[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