Bug 181739 - Update the argument count in DFGByteCodeParser::handleRecursiveCall
Summary: Update the argument count in DFGByteCodeParser::handleRecursiveCall
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-17 06:25 PST by Robin Morisset
Modified: 2018-01-23 06:28 PST (History)
7 users (show)

See Also:


Attachments
Patch (13.00 KB, patch)
2018-01-17 12:14 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (13.17 KB, patch)
2018-01-18 03:41 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (13.00 KB, patch)
2018-01-18 10:48 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (16.03 KB, patch)
2018-01-22 12:07 PST, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2018-01-17 06:25:54 PST
handleRecursiveCall can optimize tail calls with less arguments than the original caller, if we are not in an inlined call frame.
In such a situation, we need to update the argument count, so that accesses to 'arguments.length' return the right value.

It is a bit tricky, as the argument count is in a 32-bit slot on the stack, sharing its 64-bit slot with the code origin.
So I am considering either adding a new SetArgumentCountIncludingThis node, and supporting it in both DFG and FTL; or supporting PutStack in DFG (it is currently only supported in FTL) and using MovHint+PutStack, since PutStack seems to be able to store a single int32.

<rdar://problem/36120501>
Comment 1 Robin Morisset 2018-01-17 07:46:08 PST
After a second look, it appears that SetLocal can also do a 32-bit store, provided its input is marked as FlushedInt32. I will test it to see if I can avoid messing with either a new node or PutStack.
Comment 2 Robin Morisset 2018-01-17 08:59:58 PST
Phil remarked that while SetLocal and PutStack would have worked as they are currently implemented, semantically they only support 64-bit stores and so it would not be sound to use them for this purpose. So I will add a new SetArgumentCountIncludingThis node as originally suggested by Saam.
Comment 3 Robin Morisset 2018-01-17 12:14:53 PST
Created attachment 331530 [details]
Patch
Comment 4 Saam Barati 2018-01-17 12:25:37 PST
Comment on attachment 331530 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331530&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1400
> +            }

This is wrong. Please add a test. The machine argument count is not known at compile time. We must always update it.
Comment 5 Robin Morisset 2018-01-18 03:41:08 PST
Created attachment 331609 [details]
Patch
Comment 6 Robin Morisset 2018-01-18 03:42:25 PST
(In reply to Saam Barati from comment #4)
> Comment on attachment 331530 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331530&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1400
> > +            }
> 
> This is wrong. Please add a test. The machine argument count is not known at
> compile time. We must always update it.

Thanks for the catch, I had misunderstood the role of m_codeBlock->numParameters().

I've fixed it, and added a second test for this.
Comment 7 EWS Watchlist 2018-01-18 07:09:19 PST
Comment on attachment 331609 [details]
Patch

Attachment 331609 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6120464

New failing tests:
stress/inline-call-to-recursive-tail-call.js.ftl-no-cjit-no-inline-validate
stress/v8-earley-boyer-strict.js.ftl-eager-no-cjit
stress/tail-call-no-stack-overflow.js.no-cjit-validate-phases
stress/inline-call-to-recursive-tail-call.js.ftl-no-cjit-no-put-stack-validate
stress/inline-call-to-recursive-tail-call.js.dfg-maximal-flush-validate-no-cjit
stress/v8-earley-boyer-strict.js.dfg-eager-no-cjit-validate
stress/dfg-tail-calls.js.dfg-maximal-flush-validate-no-cjit
stress/async-iteration-basic.js.dfg-eager-no-cjit-validate
stress/recursive-tail-call-with-different-argument-count.js.ftl-eager-no-cjit
stress/dfg-tail-calls.js.no-cjit-validate-phases
stress/dfg-tail-calls.js.ftl-no-cjit-no-inline-validate
stress/inline-call-to-recursive-tail-call.js.ftl-eager-no-cjit
stress/inline-call-to-recursive-tail-call.js.ftl-no-cjit-validate-sampling-profiler
stress/dfg-tail-calls.js.dfg-eager-no-cjit-validate
stress/v8-earley-boyer-strict.js.no-cjit-validate-phases
stress/async-iteration-basic.js.no-cjit-validate-phases
stress/tail-call-no-stack-overflow.js.ftl-no-cjit-no-put-stack-validate
stress/recursive-tail-call-with-different-argument-count.js.ftl-eager-no-cjit-b3o1
stress/recursive-tail-call-with-different-argument-count.js.ftl-eager
stress/inline-call-to-recursive-tail-call.js.ftl-eager-no-cjit-b3o1
stress/async-iteration-async-from-sync.js.ftl-eager-no-cjit-b3o1
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-no-inline-validate
stress/async-iteration-yield-star.js.ftl-no-cjit-no-put-stack-validate
stress/inline-call-to-recursive-tail-call.js.dfg-eager-no-cjit-validate
stress/tail-call-no-stack-overflow.js.ftl-eager-no-cjit-b3o1
stress/async-iteration-basic.js.ftl-eager-no-cjit
stress/v8-earley-boyer-strict.js.dfg-maximal-flush-validate-no-cjit
stress/tail-call-no-stack-overflow.js.ftl-no-cjit-validate-sampling-profiler
stress/async-iteration-yield-promise.js.ftl-eager-no-cjit-b3o1
stress/tail-call-no-stack-overflow.js.dfg-eager-no-cjit-validate
stress/async-iteration-basic.js.dfg-maximal-flush-validate-no-cjit
stress/v8-earley-boyer-strict.js.ftl-no-cjit-no-inline-validate
stress/async-iteration-yield-star.js.ftl-eager-no-cjit-b3o1
stress/async-iteration-yield-star-interface.js.ftl-eager-no-cjit-b3o1
stress/dfg-tail-calls.js.ftl-no-cjit-no-put-stack-validate
stress/async-iteration-yield-star.js.no-cjit-validate-phases
stress/v8-earley-boyer-strict.js.ftl-no-cjit-no-put-stack-validate
stress/recursive-tail-call-with-different-argument-count.js.dfg-maximal-flush-validate-no-cjit
stress/dfg-tail-calls.js.ftl-no-cjit-validate-sampling-profiler
stress/async-iteration-async-from-sync.js.dfg-eager-no-cjit-validate
stress/tail-call-no-stack-overflow.js.ftl-no-cjit-no-inline-validate
stress/dfg-tail-calls.js.ftl-eager-no-cjit-b3o1
stress/async-iteration-yield-star.js.dfg-eager-no-cjit-validate
stress/async-iteration-async-from-sync.js.ftl-eager-no-cjit
stress/recursive-tail-call-with-different-argument-count.js.no-cjit-validate-phases
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-no-put-stack-validate
stress/dfg-tail-calls.js.ftl-eager-no-cjit
stress/async-iteration-basic.js.ftl-no-cjit-no-put-stack-validate
stress/inline-call-to-recursive-tail-call.js.ftl-eager
stress/v8-earley-boyer-strict.js.ftl-no-cjit-validate-sampling-profiler
stress/tail-call-no-stack-overflow.js.ftl-eager-no-cjit
stress/recursive-tail-call-with-different-argument-count.js.dfg-eager-no-cjit-validate
stress/async-iteration-basic.js.ftl-no-cjit-validate-sampling-profiler
stress/v8-earley-boyer-strict.js.ftl-eager-no-cjit-b3o1
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-validate-sampling-profiler
stress/inline-call-to-recursive-tail-call.js.no-cjit-validate-phases
stress/async-iteration-yield-star.js.ftl-no-cjit-validate-sampling-profiler
stress/tail-call-no-stack-overflow.js.dfg-maximal-flush-validate-no-cjit
stress/v8-earley-boyer-strict.js.default
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-b3o1
stress/async-iteration-yield-star.js.dfg-maximal-flush-validate-no-cjit
stress/async-iteration-yield-star.js.ftl-eager
stress/async-iteration-basic.js.ftl-eager-no-cjit-b3o1
Comment 8 Robin Morisset 2018-01-18 10:48:04 PST
Created attachment 331641 [details]
Patch
Comment 9 Saam Barati 2018-01-18 11:31:07 PST
Comment on attachment 331641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331641&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        It required adding a new DFG node: 'SetArgumentCountIncludingThis', that takes an unsigned int
> +        as its first OpInfo field, and stores it to the stack at the right place.

Please also add a description of the bug here. This just states what your solution is. It doesn't go into enough detail about what was wrong with the initial implementation.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1405
> +        // We must update the argument count on the stack with 'SetArgumentCountIncludingThis' instead of SetLocal,
> +        // because it is a 32-bit slot on the stack, and not a normal (64 bit) js value.

This comment does not add any real info IMO. I think it falls out just from reading the code.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:11303
> +    m_jit.store32(TrustedImm32(node->argumentCountIncludingThis()), JITCompiler::payloadFor(CallFrameSlot::argumentCount));

You need to add
noResult(node)
here.
Comment 10 Saam Barati 2018-01-18 11:32:51 PST
<rdar://problem/36627662>
Comment 11 EWS Watchlist 2018-01-18 11:34:12 PST
Comment on attachment 331641 [details]
Patch

Attachment 331641 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6123503

New failing tests:
stress/v8-earley-boyer-strict.js.ftl-eager-no-cjit
stress/recursive-tail-call-with-different-argument-count.js.ftl-eager-no-cjit
stress/inline-call-to-recursive-tail-call.js.ftl-eager-no-cjit
stress/recursive-tail-call-with-different-argument-count.js.ftl-eager-no-cjit-b3o1
stress/recursive-tail-call-with-different-argument-count.js.ftl-eager
stress/inline-call-to-recursive-tail-call.js.ftl-eager-no-cjit-b3o1
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-no-inline-validate
stress/async-iteration-basic.js.ftl-eager-no-cjit
stress/v8-earley-boyer-strict.js.ftl-no-cjit-validate-sampling-profiler
stress/async-iteration-yield-star.js.ftl-eager-no-cjit
stress/v8-earley-boyer-strict.js.ftl-no-cjit-no-inline-validate
stress/async-iteration-yield-star.js.ftl-eager-no-cjit-b3o1
stress/v8-earley-boyer-strict.js.default
stress/async-iteration-async-from-sync.js.ftl-eager-no-cjit
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-no-put-stack-validate
stress/async-iteration-yield-promise.js.ftl-eager-no-cjit-b3o1
stress/v8-earley-boyer-strict.js.ftl-eager
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-validate-sampling-profiler
stress/inline-call-to-recursive-tail-call.js.ftl-eager
stress/v8-earley-boyer-strict.js.ftl-eager-no-cjit-b3o1
stress/async-iteration-yield-star-interface.js.ftl-eager-no-cjit-b3o1
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-b3o1
stress/inline-call-to-recursive-tail-call.js.ftl-no-cjit-no-inline-validate
stress/async-iteration-yield-star.js.ftl-eager
stress/async-iteration-basic.js.ftl-eager-no-cjit-b3o1
Comment 12 Robin Morisset 2018-01-22 12:07:47 PST
Created attachment 331947 [details]
Patch
Comment 13 Saam Barati 2018-01-22 13:51:30 PST
Comment on attachment 331947 [details]
Patch

r=me
Comment 14 WebKit Commit Bot 2018-01-23 06:28:15 PST
Comment on attachment 331947 [details]
Patch

Clearing flags on attachment: 331947

Committed r227410: <https://trac.webkit.org/changeset/227410>
Comment 15 WebKit Commit Bot 2018-01-23 06:28:16 PST
All reviewed patches have been landed.  Closing bug.