Bug 158666 - Add new builtin opcode tailCallForwardArguments
Summary: Add new builtin opcode tailCallForwardArguments
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-11 12:52 PDT by Keith Miller
Modified: 2016-06-14 11:28 PDT (History)
7 users (show)

See Also:


Attachments
Patch (64.99 KB, patch)
2016-06-11 13:13 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (72.94 KB, patch)
2016-06-12 13:16 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (73.94 KB, patch)
2016-06-13 13:51 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-06-11 12:52:21 PDT
Add new builtin opcode tailCallForwardArguments
Comment 1 Keith Miller 2016-06-11 13:13:27 PDT
Created attachment 281108 [details]
Patch
Comment 2 WebKit Commit Bot 2016-06-11 13:15:32 PDT
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 3 Saam Barati 2016-06-11 15:23:07 PDT
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
Comment 4 Saam Barati 2016-06-11 15:23:34 PDT
Also looks like 32-bit needs some love.
Comment 5 Keith Miller 2016-06-11 15:31:56 PDT
(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 6 Filip Pizlo 2016-06-11 17:55:34 PDT
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 7 Keith Miller 2016-06-12 13:15:00 PDT
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.
Comment 8 Keith Miller 2016-06-12 13:16:55 PDT
Created attachment 281127 [details]
Patch
Comment 9 WebKit Commit Bot 2016-06-12 13:19:03 PDT
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.
Comment 10 Filip Pizlo 2016-06-12 15:17:29 PDT
(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 11 Filip Pizlo 2016-06-12 15:19:30 PDT
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!
Comment 12 Keith Miller 2016-06-13 13:51:26 PDT
Created attachment 281196 [details]
Patch for landing
Comment 13 Keith Miller 2016-06-13 14:04:04 PDT
Committed r202003: <http://trac.webkit.org/changeset/202003>
Comment 14 Csaba Osztrogonác 2016-06-14 03:13:51 PDT
(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.
Comment 15 Alex Christensen 2016-06-14 11:28:41 PDT
Fixed in https://bugs.webkit.org/show_bug.cgi?id=158737