Add new builtin opcode tailCallForwardArguments
Created attachment 281108 [details] Patch
Attachment 281108 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4503: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4505: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 281108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281108&action=review Some quick comments. I'll look more closely later. Pizlo might also be a better reviewer here than me > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4481 > + // Verify that handleCall(), which could have inlined the callee, didn't trash m_currentInstruction. ASSERT_WITH_MESSAGE instead of ASSERT + comment > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4497 > + ASSERT(m_currentInstruction == currentInstruction); ASSERT_WITH_MESSAGE instead of ASSERT + comment > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4499 > + // If the call is not terminal, however, then we want the subsequent op_ret/op_jumpt to update metadata and clean Typo: jumpt > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4501 > + if (terminality == NonTerminal) { When is this true? When this is inlined? > Source/JavaScriptCore/jit/JITOperations.cpp:1937 > +CallFrame* JIT_OPERATION operationSetupForwardArgumentsFrame(ExecState* exec, CallFrame* newCallFrame, EncodedJSValue, int32_t, int32_t length) Might as well reuse this same code you wrote in Interpreter.cpp and you can just inline it if you don't want to pay for the call. > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:749 > +macro prepareForTailCallForwardArguments(callee, temp1, temp2, temp3) Not used
Also looks like 32-bit needs some love.
(In reply to comment #4) > Also looks like 32-bit needs some love. Yeah, I'll take a look at that a little later.
Comment on attachment 281108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281108&action=review > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1250 > + ASSERT(callNode->op() != TailCallVarargs || callNode->op() != TailCallForwardVarargs); Isn't this a tautology? X is always my equal to A or not equal to B if A and B are not equal to each other. Maybe &&? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1778 > + noticeArgumentsUse(); Wow, that's tricky! >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4501 >> + if (terminality == NonTerminal) { > > When is this true? When this is inlined? I guess foo normal-called bar and bar tail-forward-called baz? If that's it, then having such an example in the comment would help. > Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h:120 > + if (m_node->child1()) This seems like the wrong condition since I'm assuming that child1 is the arguments child some of the time and the callee child for the call opcodes. Seems like this was already a bug.
Comment on attachment 281108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281108&action=review >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1250 >> + ASSERT(callNode->op() != TailCallVarargs || callNode->op() != TailCallForwardVarargs); > > Isn't this a tautology? X is always my equal to A or not equal to B if A and B are not equal to each other. Maybe &&? You're right that should be &&. I'll fix all the cases of this. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4481 >> + // Verify that handleCall(), which could have inlined the callee, didn't trash m_currentInstruction. > > ASSERT_WITH_MESSAGE instead of ASSERT + comment Fixed. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4497 >> + ASSERT(m_currentInstruction == currentInstruction); > > ASSERT_WITH_MESSAGE instead of ASSERT + comment Fixed. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4499 >> + // If the call is not terminal, however, then we want the subsequent op_ret/op_jumpt to update metadata and clean > > Typo: jumpt Fixed along with the other similar comments. >>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4501 >>> + if (terminality == NonTerminal) { >> >> When is this true? When this is inlined? > > I guess foo normal-called bar and bar tail-forward-called baz? > > If that's it, then having such an example in the comment would help. This happens when whenever the call is inlined. I believe the caller does not need to be inlined for this to happen. >> Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h:120 >> + if (m_node->child1()) > > This seems like the wrong condition since I'm assuming that child1 is the arguments child some of the time and the callee child for the call opcodes. Seems like this was already a bug. Yeah, I think you are right. I missed that this code was working on both ForwardVarargs and TailCallForwardVarargs. >> Source/JavaScriptCore/jit/JITOperations.cpp:1937 >> +CallFrame* JIT_OPERATION operationSetupForwardArgumentsFrame(ExecState* exec, CallFrame* newCallFrame, EncodedJSValue, int32_t, int32_t length) > > Might as well reuse this same code you wrote in Interpreter.cpp and you can just inline it if you don't want to pay for the call. I meant to do that but I guess I forgot to update this code. >> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:749 >> +macro prepareForTailCallForwardArguments(callee, temp1, temp2, temp3) > > Not used Removed.
Created attachment 281127 [details] Patch
Attachment 281127 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4501: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4503: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #7) > Comment on attachment 281108 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281108&action=review > > >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1250 > >> + ASSERT(callNode->op() != TailCallVarargs || callNode->op() != TailCallForwardVarargs); > > > > Isn't this a tautology? X is always my equal to A or not equal to B if A and B are not equal to each other. Maybe &&? > > You're right that should be &&. I'll fix all the cases of this. > > >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4481 > >> + // Verify that handleCall(), which could have inlined the callee, didn't trash m_currentInstruction. > > > > ASSERT_WITH_MESSAGE instead of ASSERT + comment > > Fixed. > > >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4497 > >> + ASSERT(m_currentInstruction == currentInstruction); > > > > ASSERT_WITH_MESSAGE instead of ASSERT + comment > > Fixed. > > >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4499 > >> + // If the call is not terminal, however, then we want the subsequent op_ret/op_jumpt to update metadata and clean > > > > Typo: jumpt > > Fixed along with the other similar comments. > > >>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4501 > >>> + if (terminality == NonTerminal) { > >> > >> When is this true? When this is inlined? > > > > I guess foo normal-called bar and bar tail-forward-called baz? > > > > If that's it, then having such an example in the comment would help. > > This happens when whenever the call is inlined. I believe the caller does > not need to be inlined for this to happen. If we have foo tail-forward calling bar, then bar should a terminal inline. Right? > > >> Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h:120 > >> + if (m_node->child1()) > > > > This seems like the wrong condition since I'm assuming that child1 is the arguments child some of the time and the callee child for the call opcodes. Seems like this was already a bug. > > Yeah, I think you are right. I missed that this code was working on both > ForwardVarargs and TailCallForwardVarargs. > > >> Source/JavaScriptCore/jit/JITOperations.cpp:1937 > >> +CallFrame* JIT_OPERATION operationSetupForwardArgumentsFrame(ExecState* exec, CallFrame* newCallFrame, EncodedJSValue, int32_t, int32_t length) > > > > Might as well reuse this same code you wrote in Interpreter.cpp and you can just inline it if you don't want to pay for the call. > > I meant to do that but I guess I forgot to update this code. > > >> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:749 > >> +macro prepareForTailCallForwardArguments(callee, temp1, temp2, temp3) > > > > Not used > > Removed.
Comment on attachment 281127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281127&action=review r=me. Make sure you test everything. > Source/JavaScriptCore/dfg/DFGNode.h:774 > + Edge& argumentsChild() Nice!
Created attachment 281196 [details] Patch for landing
Committed r202003: <http://trac.webkit.org/changeset/202003>
(In reply to comment #13) > Committed r202003: <http://trac.webkit.org/changeset/202003> It broke many JSC tests on 32 bit platforms, see build.webkit.org for details. Apple El Capitan 32-bit JSC (BuildAndTest) - before: https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/2649 - after: https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/2650 Apple Yosemite 32-bit JSC (BuildAndTest) - before: https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/9462 - after: https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/9463 Apple Win 7 Release (Tests) - before: https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/58209 - after: https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/58210 JSCOnly Linux ARMv7 Thumb2 Release - before: https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/935 - after: https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/936 JSCOnly Linux ARMv7 Traditional Release - before: https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Traditional%20Release/builds/931 - after: https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Traditional%20Release/builds/932 Please watch the build waterfall after landing a patch.
Fixed in https://bugs.webkit.org/show_bug.cgi?id=158737