Air should allocate registers

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Instead of spilling everything.

[Benjamin Poulain]

Created attachment 265257 [details]
Patch

[Filip Pizlo]

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.

[Benjamin Poulain]

Committed r192292: <http://trac.webkit.org/changeset/192292>

[Filip Pizlo]

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?

[Filip Pizlo]

(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