Last Comment Bug 711763 - IonMonkey: Simplify the greedy allocator
: IonMonkey: Simplify the greedy allocator
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
Mentors:
: 706896 708757 (view as bug list)
Depends on:
Blocks: 695075
  Show dependency treegraph
 
Reported: 2011-12-17 13:15 PST by David Anderson [:dvander]
Modified: 2011-12-27 17:44 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP v0 (30.69 KB, patch)
2011-12-17 13:15 PST, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
patch (50.07 KB, patch)
2011-12-19 17:36 PST, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
v2 (43.83 KB, patch)
2011-12-20 16:12 PST, David Anderson [:dvander]
sstangl: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2011-12-17 13:15:57 PST
Created attachment 582585 [details] [diff] [review]
WIP v0

I'm still not sure whether this allocator is useful, but it's really complex (arguably, moreso than LSRA) and has lots of bugs. This patch simplifies it by removing all the control flow handling code and instead spilling the register state at the top of basic blocks. It still gets good allocation inside a basic block.

(Note, this patch is totally untested.)
Comment 1 David Anderson [:dvander] 2011-12-19 17:36:31 PST
Created attachment 583034 [details] [diff] [review]
patch

 7 files changed, 212 insertions(+), 624 deletions(-)

This also fixes a bunch of test failures, so I guess the old implementation was actually pretty buggy.
Comment 2 David Anderson [:dvander] 2011-12-20 16:12:12 PST
Created attachment 583331 [details] [diff] [review]
v2

This version is a little less aggressive. It keeps FindNaturalLoops which makes safepoint integration much easier.
Comment 3 David Anderson [:dvander] 2011-12-22 11:37:06 PST
*** Bug 706896 has been marked as a duplicate of this bug. ***
Comment 4 David Anderson [:dvander] 2011-12-22 16:27:17 PST
*** Bug 708757 has been marked as a duplicate of this bug. ***
Comment 5 Sean Stangl [:sstangl] 2011-12-27 16:23:27 PST
Comment on attachment 583331 [details] [diff] [review]
v2

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

This looks great. It's significantly simpler. Haven't found any issues.

::: js/src/ion/C1Spewer.cpp
@@ +55,5 @@
>  
>  bool
>  C1Spewer::init(const char *path)
>  {
> +    spewout_ = NULL; //fopen(path, "w");

Hm.

::: js/src/ion/MIRGraph.h
@@ +406,5 @@
>          JS_ASSERT(isLoopHeader());
>          return containedInLoop_.append(block);
>      }
>  
> +	void setLoopDepth(uint32 loopDepth) {

One indentation level back
Comment 6 David Anderson [:dvander] 2011-12-27 17:44:35 PST
http://hg.mozilla.org/projects/ionmonkey/rev/3725545a3e35

Note You need to log in before you can comment on or make changes to this bug.