...
While FTL can drop this, it is good if we can avoid this thing in LLInt, Baseline and DFG.
It seems that this will give us 7.4% improvement in ES6SampleBench/Basic! 164.36ms vs 152.98ms
Created attachment 294187 [details] Patch WIP
Comment on attachment 294187 [details] Patch Attachment 294187 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2479692 New failing tests: js/array-fill.html
Created attachment 294195 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 294187 [details] Patch Attachment 294187 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2479697 New failing tests: js/array-fill.html
Created attachment 294196 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 294197 [details] Patch WIP
Related https://bugs.webkit.org/show_bug.cgi?id=143077
Created attachment 294210 [details] Patch WIP
Created attachment 294211 [details] Patch WIP
Comment on attachment 294211 [details] Patch Attachment 294211 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2480705 Number of test failures exceeded the failure limit.
Created attachment 294213 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 294214 [details] Patch WIP
Created attachment 294215 [details] Patch
The patch is ready.
Comment on attachment 294215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294215&action=review r=me However, I'd like to see some tests for varargs inlined callees. > Source/JavaScriptCore/ChangeLog:60 > + All summary. > + Baseline: > + summary: 164.34 ms +- 5.01 ms > + Patched: > + summary: 157.26 ms +- 5.96 ms Nice! > Source/JavaScriptCore/builtins/ArrayPrototype.js:314 > + var relativeStart = @toInteger(@argument(1)); toInteger of undefined is zero? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6784 > + JSValueRegs resultRegs = result.regs(); > + m_jit.load32(CCallHelpers::payloadFor(CallFrameSlot::argumentCount), argumentCountGPR); This doesn't seem correct if we've been inlined. Can you add tests? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6785 > + auto argumentOutOfBounds = m_jit.branch32(CCallHelpers::LessThanOrEqual, argumentCountGPR, CCallHelpers::Imm32(node->argumentIndex())); Nit: This could be TrustedImm32 since it comes from our builtins. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5168 > + LValue argumentCount = m_out.load32(payloadFor(CallFrameSlot::argumentCount)); Ditto, this does't look correct for inlining (I guess varargs inlinled callee is when this could come up because of your bytecode parser rules) > Source/JavaScriptCore/jit/JITOpcodes.cpp:511 > + Jump argumentOutOfBounds = branch32(LessThanOrEqual, regT0, Imm32(index)); Ditto nit: Could be TrustedImm32
Comment on attachment 294215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294215&action=review > Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h:112 > + case GetArgument: Please make this rule more specific.
Comment on attachment 294215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294215&action=review Thanks! >> Source/JavaScriptCore/ChangeLog:60 >> + summary: 157.26 ms +- 5.96 ms > > Nice! Yeah! >> Source/JavaScriptCore/builtins/ArrayPrototype.js:314 >> + var relativeStart = @toInteger(@argument(1)); > > toInteger of undefined is zero? Yes >> Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h:112 >> + case GetArgument: > > Please make this rule more specific. OK, fixed. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6784 >> + m_jit.load32(CCallHelpers::payloadFor(CallFrameSlot::argumentCount), argumentCountGPR); > > This doesn't seem correct if we've been inlined. Can you add tests? Yeah, fixed. And I've added tests for that. Add a new helper function AssemblyHelpers::argumentCount(const CodeOrigin&). >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6785 >> + auto argumentOutOfBounds = m_jit.branch32(CCallHelpers::LessThanOrEqual, argumentCountGPR, CCallHelpers::Imm32(node->argumentIndex())); > > Nit: This could be TrustedImm32 since it comes from our builtins. Nice, fixed. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6791 > +#endif And use AssemblyHelpers::argumentsStart for that. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5168 >> + LValue argumentCount = m_out.load32(payloadFor(CallFrameSlot::argumentCount)); > > Ditto, this does't look correct for inlining (I guess varargs inlinled callee is when this could come up because of your bytecode parser rules) Fixed. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5177 > + ValueFromBlock inBoundsResult = m_out.anchor(m_out.load64(payloadFor(CallFrameSlot::thisArgument + m_node->argumentIndex()))); And use AssemblyHelpers::argumentsStart for that. >> Source/JavaScriptCore/jit/JITOpcodes.cpp:511 >> + Jump argumentOutOfBounds = branch32(LessThanOrEqual, regT0, Imm32(index)); > > Ditto nit: Could be TrustedImm32 Fixed.
Committed r208524: <http://trac.webkit.org/changeset/208524>