1136898 - IonMonkey: remove LinearScan

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: remove-lsra.patch

With the backtracking allocator now enabled by default (bug 826741), we can remove the LinearScan allocator. This also allows us to remove LLabel, LNop, and even LMop (bug 1096138), as well as simplify the liveness computation code in LiveRangeAllocator.cpp.

Comment 1

[Jan de Mooij [:jandem]]

This is awesome, but I think it'd be good to wait at least until next week so we can switch back if we find some serious issues on Nightly. I can start reviewing the patch though.

Comment 2

[Luke Wagner [:luke]]

Random naming suggestion: since there would no longer be two (serious) allocators, could we globally rename "BacktrackingAllocator" to just "RegisterAllocator" (keeping StupidAllocator as is)?  I'm guessing we won't be landing any new experimental allocators for a while...

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #2)
> Random naming suggestion: since there would no longer be two (serious)
> allocators, could we globally rename "BacktrackingAllocator" to just
> "RegisterAllocator" (keeping StupidAllocator as is)?  I'm guessing we won't
> be landing any new experimental allocators for a while...

There is already a RegisterAllocator class which has code common to the backtracking and stupid allocators.  I think after this lands we should fold LiveRangeAllocator and its template mumbo jumbo into BacktrackingAllocator and remove LiveRangeAllocator, but it would be kind of nice to keep the name for BacktrackingAllocator to isolate its function from the more generic stuff.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

\o/

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8569416 [details] [diff] [review]
remove-lsra.patch

Review of attachment 8569416 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful, removes even more code than I expected :) Please wait until Monday or so, in case BT happens to be crashy on some popular websites.

::: js/src/jit/CodeGenerator.cpp
@@ -4283,5 @@
>      iter++;
>  
>      while (true) {
>          for (; iter != block->end(); iter++) {
> -            if (iter->isNop() || iter->isConstant() || iter->isPostWriteBarrier()) {

We're checking the MIR graph here so we should keep the iter->isNop() check. MNops are still inserted in some cases.

Comment 6

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> This is awesome, but I think it'd be good to wait at least until next week
> so we can switch back if we find some serious issues on Nightly. I can start
> reviewing the patch though.

I thought we would keep it for 1/2 months, making sure that if something serious is thrown our way, we have the option to disable it when it is already in aurora/beta.

Comment 7

[Dan Gohman [:sunfish]]

(In reply to Hannes Verschore [:h4writer] from comment #6)
> (In reply to Jan de Mooij [:jandem] from comment #1)
> > This is awesome, but I think it'd be good to wait at least until next week
> > so we can switch back if we find some serious issues on Nightly. I can start
> > reviewing the patch though.
> 
> I thought we would keep it for 1/2 months, making sure that if something
> serious is thrown our way, we have the option to disable it when it is
> already in aurora/beta.

How about this: if something serious is thrown our way, and we can't fix it in backtracking, I volunteer to port LinearScan up to trunk and re-land it.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: rebase (40723ea7e393)

Rebased patch.  It would be cool if this could land soon, as I'd like to do this before bug 1067610 so that bug 1067610 can have a cleaner patch that restructures the backtracking allocator more fundamentally.

Comment 9

[Dan Gohman [:sunfish]]

jandem, are you ok with this rebased patch landing at this time?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Dan Gohman [:sunfish] from comment #9)
> jandem, are you ok with this rebased patch landing at this time?

Seems fine, thanks for doing this!

Comment 11

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5da7f3f0c559

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5da7f3f0c559