Closed Bug 1136898 Opened 10 years ago Closed 10 years ago

IonMonkey: remove LinearScan

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(2 files)

Attached patch remove-lsra.patch (deleted) — Splinter Review
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.
Blocks: 1136551
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.
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.
(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.
Attached patch rebase (40723ea7e393) (deleted) — Splinter Review
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.
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)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: