IonMonkey: remove LinearScan

RESOLVED FIXED in Firefox 40

Status

()

Core
JavaScript Engine: JIT
--
enhancement
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
Created attachment 8569416 [details] [diff] [review]
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.
Attachment #8569416 - Flags: review?(jdemooij)
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.
(Assignee)

Updated

3 years ago
Blocks: 1136551

Comment 2

3 years ago
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...
(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.
\o/
Blocks: 1136104
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.
Attachment #8569416 - Flags: review?(jdemooij) → review+
(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.
(Assignee)

Comment 7

3 years ago
(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.
Created attachment 8592337 [details] [diff] [review]
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.
(Assignee)

Comment 9

3 years ago
jandem, are you ok with this rebased patch landing at this time?
Flags: needinfo?(jdemooij)
(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!
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/5da7f3f0c559
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1136551
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1136104
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1115330
You need to log in before you can comment on or make changes to this bug.