Bug 158666 – Add new builtin opcode tailCallForwardArguments

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Add new builtin opcode tailCallForwardArguments

[Keith Miller]

Created attachment 281108 [details]
Patch

[WebKit Commit Bot]

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.

[Saam Barati]

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

[Saam Barati]

Also looks like 32-bit needs some love.

[Keith Miller]

(In reply to comment #4)
> Also looks like 32-bit needs some love.

Yeah, I'll take a look at that a little later.

[Filip Pizlo]

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.

[Keith Miller]

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.

[Keith Miller]

Created attachment 281127 [details]
Patch

[WebKit Commit Bot]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

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!

[Keith Miller]

Created attachment 281196 [details]
Patch for landing

[Keith Miller]

Committed r202003: <http://trac.webkit.org/changeset/202003>

[Csaba Osztrogonác]

(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.

[Alex Christensen]

Fixed in https://bugs.webkit.org/show_bug.cgi?id=158737