Bug 169820 - Air should coalesce spill slots
Summary: Air should coalesce spill slots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 169912
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-17 11:04 PDT by Filip Pizlo
Modified: 2017-03-25 11:10 PDT (History)
7 users (show)

See Also:


Attachments
work in progress (6.97 KB, patch)
2017-03-17 11:05 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it coalesced some spills (15.45 KB, patch)
2017-03-17 14:27 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
HUGE speed-up (15.44 KB, patch)
2017-03-17 16:02 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (22.20 KB, patch)
2017-03-17 19:36 PDT, Filip Pizlo
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (1.92 MB, application/zip)
2017-03-17 21:09 PDT, Build Bot
no flags Details
the patch (23.06 KB, patch)
2017-03-17 22:16 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (24.80 KB, patch)
2017-03-19 15:26 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2017-03-17 11:04:19 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2017-03-17 11:05:38 PDT
Created attachment 304798 [details]
work in progress
Comment 2 Filip Pizlo 2017-03-17 14:27:18 PDT
Created attachment 304824 [details]
it coalesced some spills

Still trying to figure out if it works for reals.
Comment 3 Filip Pizlo 2017-03-17 16:02:55 PDT
Created attachment 304832 [details]
HUGE speed-up

But it still fails some internal tests.
Comment 4 Filip Pizlo 2017-03-17 19:36:47 PDT
Created attachment 304848 [details]
the patch
Comment 5 Build Bot 2017-03-17 21:09:29 PDT
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.
Comment 6 Build Bot 2017-03-17 21:09:34 PDT
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
Comment 7 Filip Pizlo 2017-03-17 21:14:04 PDT
(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...
Comment 8 Filip Pizlo 2017-03-17 22:16:11 PDT
Created attachment 304862 [details]
the patch

Should pass debug tests now.
Comment 9 Filip Pizlo 2017-03-19 15:26:15 PDT
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 10 Michael Saboff 2017-03-20 11:29:58 PDT
Comment on attachment 304903 [details]
the patch

r=me
Comment 11 WebKit Commit Bot 2017-03-20 11:59:03 PDT
Comment on attachment 304903 [details]
the patch

Clearing flags on attachment: 304903

Committed r214187: <http://trac.webkit.org/changeset/214187>
Comment 12 WebKit Commit Bot 2017-03-20 11:59:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Saam Barati 2017-03-22 18:33:52 PDT
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.
Comment 14 Filip Pizlo 2017-03-25 11:10:36 PDT
(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.