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>
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.
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.
Created attachment 331530 [details] Patch
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.
Created attachment 331609 [details] Patch
(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 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
Created attachment 331641 [details] Patch
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.
<rdar://problem/36627662>
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
Created attachment 331947 [details] Patch
Comment on attachment 331947 [details] Patch r=me
Comment on attachment 331947 [details] Patch Clearing flags on attachment: 331947 Committed r227410: <https://trac.webkit.org/changeset/227410>
All reviewed patches have been landed. Closing bug.