Bug 169820

Created attachment 304798 [details] work in progress

Created attachment 304824 [details] it coalesced some spills Still trying to figure out if it works for reals.

Created attachment 304832 [details] HUGE speed-up But it still fails some internal tests.

Created attachment 304848 [details] the patch

Comment on attachment 304848 [details] the patch Attachment 304848 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3353666 Number of test failures exceeded the failure limit.

Created attachment 304858 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6

(In reply to comment #6) > Created attachment 304858 [details] > Archive of layout-test-results from ews114 for mac-elcapitan > > The attached test failures were seen while running run-webkit-tests on the > mac-debug-ews. > Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6 The fix is to remove the assertion. Testing this now...

Created attachment 304862 [details] the patch Should pass debug tests now.

Created attachment 304903 [details] the patch This fixes the compile time regression. At first I thought that was because coalescing was expensive, but then I found that it was simply because I introduced a bug into padInterference. It was inserting padding Nops all over the place like crazy!

Comment on attachment 304903 [details] the patch r=me

Comment on attachment 304903 [details] the patch Clearing flags on attachment: 304903 Committed r214187: <http://trac.webkit.org/changeset/214187>

All reviewed patches have been landed. Closing bug.

Comment on attachment 304903 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=304903&action=review LGTM too > Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:215 > + Vector<CoalescableMove> coalescableMoves; Style nit: I would have called this something like coalescingCandidates or something to indicate that not every move in the vector is truly coalescable.

(In reply to Saam Barati from comment #13) > Comment on attachment 304903 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=304903&action=review > > LGTM too > > > Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:215 > > + Vector<CoalescableMove> coalescableMoves; > > Style nit: I would have called this something like coalescingCandidates or > something to indicate that not every move in the vector is truly coalescable. I think of the three-operand move instructions that this patch introduces as coalescable moves, because you have to use these kinds of moves if you want to make two spill slots coalescable. I don't want to call these instucctions coalescing candidates. A pair of tmps or spill slots is a coalescing candidate. An instruction is not a coalescing candidate because we're not coalescing instructions. This data structure does not track pairs of spill slots as much as it tracks actual move instructions (because in addition to the pair of spill slots it tracks the frequency of that instruction). This makes me realize that this data structure should have the total frequency of all moves for a pair of spill slots. That would be more precise. If I make that change, then this data structure would no longer have the word "moves" in it because it would no longer track individual moves. But for now the name seems accurate.