Bug 171131 - Air::fixObviousSpills should remove totally redundant instructions
Summary: Air::fixObviousSpills should remove totally redundant instructions
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:
Blocks:
 
Reported: 2017-04-21 12:48 PDT by Filip Pizlo
Modified: 2017-05-01 14:17 PDT (History)
5 users (show)

See Also:


Attachments
the patch (10.89 KB, patch)
2017-04-21 12:54 PDT, Filip Pizlo
saam: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (11.48 MB, application/zip)
2017-04-21 15:23 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2017-04-21 12:48:57 PDT
If you know that (blah) and %foo are n-bit aliased and you do an n-bit move from (blah) to %foo or vice-versa, then you can remove the move.

If you know that (blah) and $42 are n-bit aliased and you do an n-bit move from $42 to (blah), then you can remove the move.

If you know that %foo and $42 are n-bit aliased and you do an n-bit move from $42 to %foo, then you can remove the move.
Comment 1 Filip Pizlo 2017-04-21 12:54:23 PDT
Created attachment 307775 [details]
the patch
Comment 2 Saam Barati 2017-04-21 13:17:43 PDT
Comment on attachment 307775 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307775&action=review

> Source/JavaScriptCore/b3/air/AirFixObviousSpills.cpp:201
> +        bool shouldLive = true;
> +        forAllAliases(
> +            [&] (const auto& alias) {
> +                shouldLive &= !m_state.contains(alias);
> +            });

Why is this correct? shouldLive is false if m_state.contains(alias) returns true for any alias. But don't we want to prove something stronger, that we want to keep the Inst if any alias is not in m_state?
Comment 3 Filip Pizlo 2017-04-21 13:41:33 PDT
(In reply to Saam Barati from comment #2)
> Comment on attachment 307775 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307775&action=review
> 
> > Source/JavaScriptCore/b3/air/AirFixObviousSpills.cpp:201
> > +        bool shouldLive = true;
> > +        forAllAliases(
> > +            [&] (const auto& alias) {
> > +                shouldLive &= !m_state.contains(alias);
> > +            });
> 
> Why is this correct? shouldLive is false if m_state.contains(alias) returns
> true for any alias. 

Yes.  It's correct because the aliases are intersected with each other.  Think of it this way.  The empty set of aliases is your TOP state.  Any alias you add is read as TOP && yourAlias.  The full list of aliases (A, B, C) can be read as TOP && A && B && C.  BOTTOM can only be expressed by having two aliases that contradict each other.

> But don't we want to prove something stronger, that we
> want to keep the Inst if any alias is not in m_state?

Look at the cases where we add multiple aliases.  Here's an example: we might add both a RegConst and a SlotConst alias in cases where we store a reg to a slot and the reg is known to have a constant value.

In that case, if we prove either that the slot already has that constant value or that the slot already has the current value of that register, then the instruction is not useful.
Comment 4 Saam Barati 2017-04-21 13:45:42 PDT
Comment on attachment 307775 [details]
the patch

Gotcha. Thanks for explaining.
r=me
Comment 5 Build Bot 2017-04-21 15:23:05 PDT
Comment on attachment 307775 [details]
the patch

Attachment 307775 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3578386

New failing tests:
compositing/absolute-inside-out-of-view-fixed.html
Comment 6 Build Bot 2017-04-21 15:23:07 PDT
Created attachment 307818 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 7 Filip Pizlo 2017-05-01 14:17:54 PDT
Landed in http://trac.webkit.org/changeset/216027/webkit