Bug 171131 – Air::fixObviousSpills should remove totally redundant instructions

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 307775 [details]
the patch

[Saam Barati]

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?

[Filip Pizlo]

(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.

[Saam Barati]

Comment on attachment 307775 [details]
the patch

Gotcha. Thanks for explaining.
r=me

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/216027/webkit