Bug 150457 - Air should allocate registers
Summary: Air should allocate registers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks: 150456
  Show dependency treegraph
 
Reported: 2015-10-22 11:13 PDT by Filip Pizlo
Modified: 2016-10-16 14:41 PDT (History)
2 users (show)

See Also:


Attachments
Patch (49.07 KB, patch)
2015-11-10 18:33 PST, Benjamin Poulain
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2015-10-22 11:13:41 PDT
Instead of spilling everything.
Comment 1 Benjamin Poulain 2015-11-10 18:33:13 PST
Created attachment 265257 [details]
Patch
Comment 2 Filip Pizlo 2015-11-10 18:45:58 PST
Comment on attachment 265257 [details]
Patch

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

> Source/JavaScriptCore/b3/air/AirInstInlines.h:63
> -                arg = Arg::stack(stackSlot);
> +                arg = Arg::stack(stackSlot, arg.offset());

Oops.

> Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:51
> +        bool isMove = inst.opcode == Move || inst.opcode == Move32;

We shouldn't coalesce Move32's, since they zero-extend.
Comment 3 Benjamin Poulain 2015-11-10 21:10:39 PST
Committed r192292: <http://trac.webkit.org/changeset/192292>
Comment 4 Filip Pizlo 2016-10-16 13:48:22 PDT
Comment on attachment 265257 [details]
Patch

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

> Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:323
> +            if (!degree)
> +                continue;

Oh man!  This finally turned into a bug, I think!  A spill tmp from the previous iteration may have no degree: imagine a tmp that is live everywhere and interferes with everyone, but has one use like:

Move %ourTmp, %someOtherTmp

Where there are no other tmps live.  After spill conversion, this may look like:

Move (ourSpill), %newTmp
Move %newTmp, %someOtherTmp

Of course, we'd rather not get this kind of spill code but it's totally possible because we now have a bunch of random conditions under which we won't slap the spill address directly into the Move.

After this happens, assuming that the only thing live was %someOtherTmp, we will have zero degree for %newTmp because the Move is coalescable and does not contribute to interference.

Then, we might coalesce %someOtherTmp with %newTmp.  Once this happens, if we make the %newTmp be the master, we're in deep trouble because %newTmp is not on any worklist due to these two lines of code!

Do you agree that these two lines can be removed?
Comment 5 Filip Pizlo 2016-10-16 14:41:19 PDT
(In reply to comment #4)
> Comment on attachment 265257 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265257&action=review
> 
> > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:323
> > +            if (!degree)
> > +                continue;
> 
> Oh man!  This finally turned into a bug, I think!  A spill tmp from the
> previous iteration may have no degree: imagine a tmp that is live everywhere
> and interferes with everyone, but has one use like:
> 
> Move %ourTmp, %someOtherTmp
> 
> Where there are no other tmps live.  After spill conversion, this may look
> like:
> 
> Move (ourSpill), %newTmp
> Move %newTmp, %someOtherTmp
> 
> Of course, we'd rather not get this kind of spill code but it's totally
> possible because we now have a bunch of random conditions under which we
> won't slap the spill address directly into the Move.
> 
> After this happens, assuming that the only thing live was %someOtherTmp, we
> will have zero degree for %newTmp because the Move is coalescable and does
> not contribute to interference.
> 
> Then, we might coalesce %someOtherTmp with %newTmp.  Once this happens, if
> we make the %newTmp be the master, we're in deep trouble because %newTmp is
> not on any worklist due to these two lines of code!
> 
> Do you agree that these two lines can be removed?

https://bugs.webkit.org/show_bug.cgi?id=163509