Currently, when executing the following code, var array = [0, 1, 2]; Math.max(...array); The bytecode becomes like this. [ 49] spread loc9, loc10 [ 52] new_array_with_spread loc7, loc9, 1, BitVector:0:1 [ 57] mov loc9, loc8 [ 60] call_varargs loc5, loc5, loc8, loc7, -11, 0 predicting None But (52)'s array is unnecessary if argument is only consisted by one spread.
Created attachment 307595 [details] Patch WIP
Comment on attachment 307595 [details] Patch I was thinking about doing something like this when I added JSFixedArray. I think this is probably best done as a transformation over DFG IR. If you can prove the array hasn't changed, you can pass the JSFixedArray directly in. That said, maybe it's worth setting up the infrastructure with array literals so it can be done in the bytecode generator. But I think the most success would be to do this over DFG IR
(In reply to Saam Barati from comment #2) > Comment on attachment 307595 [details] > Patch > > I was thinking about doing something like this when I added JSFixedArray. I > think this is probably best done as a transformation over DFG IR. If you can > prove the array hasn't changed, you can pass the JSFixedArray directly in. > That said, maybe it's worth setting up the infrastructure with array > literals so it can be done in the bytecode generator. But I think the most > success would be to do this over DFG IR Yeah, that's nice insight! Thanks, eliminating NewArrayWithSpread in (maybe arguments elimination phase or object allocation sinking phase) seems clean. (And the current patch is lacking this phase change right now b/c of WIP :P) But I'm also exploring this approach because it is beneficial in all the tiers while arguments elimination phase is only done in FTL. And I'm also considering about CallVararg(RuntimeFunc, Spread). Now, it's still rough design, I need to look into this deeply.
(In reply to Yusuke Suzuki from comment #3) > Yeah, that's nice insight! Thanks, eliminating NewArrayWithSpread in (maybe > arguments elimination phase or object allocation sinking phase) seems clean. > (And the current patch is lacking this phase change right now b/c of WIP :P) > > But I'm also exploring this approach because it is beneficial in all the > tiers while arguments elimination phase is only done in FTL. > And I'm also considering about CallVararg(RuntimeFunc, Spread). > > Now, it's still rough design, I need to look into this deeply. OK, I would like to do the both. By only emitting op_spread, we can eliminate the new_array_with_spread for call_varargs in all the tiers. And we also would like to relax the current limiation for NewArrayWithSpread in arguments elimination phase to accept non-phantom spread & non-create-rest-related spread.
(In reply to Yusuke Suzuki from comment #4) > OK, I would like to do the both. By only emitting op_spread, we can > eliminate the new_array_with_spread for call_varargs in all the tiers. And > we also would like to relax the current limiation for NewArrayWithSpread in > arguments elimination phase to accept non-phantom spread & > non-create-rest-related spread. Simple bytecompiler level one will optimize the case like. call(...array) It's super easy to do that, and it is super beneficial because it is effective even in LLInt. And the above call form is quite common, so I think it's worth doing in bytecompiler. And the code itself should be quite simple, just adding small code to BytecodeGenerator. (And support spread form varargs.) But we would like to analyze dfg and optimize, var array = [...values]; call(...array); thing too. Such thing can be effectively done in DFG arguments elimination phase.
(In reply to Yusuke Suzuki from comment #5) > (In reply to Yusuke Suzuki from comment #4) > > OK, I would like to do the both. By only emitting op_spread, we can > > eliminate the new_array_with_spread for call_varargs in all the tiers. And > > we also would like to relax the current limiation for NewArrayWithSpread in > > arguments elimination phase to accept non-phantom spread & > > non-create-rest-related spread. > > Simple bytecompiler level one will optimize the case like. > > call(...array) > > It's super easy to do that, and it is super beneficial because it is > effective even in LLInt. And the above call form is quite common, so I think > it's worth doing in bytecompiler. And the code itself should be quite > simple, just adding small code to BytecodeGenerator. (And support spread > form varargs.) > > But we would like to analyze dfg and optimize, > > var array = [...values]; > call(...array); > > thing too. Such thing can be effectively done in DFG arguments elimination > phase. Yeah I agree. I think it's worth having this peephole optimization in the byte compiler. We can develop a more sophisticated analysis in the DFG later.
Created attachment 307714 [details] Patch WIP
Comment on attachment 307714 [details] Patch Attachment 307714 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3575475 Number of test failures exceeded the failure limit.
Created attachment 307717 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 307908 [details] Patch
Comment on attachment 307908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307908&action=review nice. r=me. I think once we implement a more sophisticated analysis to transform code into this, it'll be nice to have this support in the FTL. > Source/JavaScriptCore/ChangeLog:12 > + to oit new_array_with_spread. This is very simple and effective because this typo: oit => emit
Comment on attachment 307908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307908&action=review >> Source/JavaScriptCore/ChangeLog:12 >> + to oit new_array_with_spread. This is very simple and effective because this > > typo: oit => emit Ah, this is typo, we should fix it oit => omit. Thanks!
Committed r215720: <http://trac.webkit.org/changeset/215720>