Implement basic support for deferred slow path code with calls that save and restore live registers.

Implement basic support for deferred slow path code with calls that save and restore live registers.

Use it to avoid spilling over stack-checks.

R=kmillikin@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=10639

[Kevin Millikin (Google), 2012-08-14 11:18:16]

LGTM except for some naming suggestions.

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_allocator.cc (right):

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.cc:315: SafepointPosition* new_pos = new
SafepointPosition(pos, locs, NULL);
Weird that new_pos has a different type than pos.  Suggestion new_pos =>
safepoint.

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.cc:322: ASSERT(safepoints_tail_->pos() < pos);
Comment that we assume the list is sorted by position when splitting it.

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_allocator.h (right):

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.h:422: safepoints_(NULL),
I think the insertion code reads better if you change safepoints_ =>
first_safepoint_ and safepoints_tail_ => last_safepoint_.

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/locations.h
File runtime/vm/locations.h (right):

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/locations.h...
runtime/vm/locations.h:298: ContainsCall contains_call() const {
It sounds like this returns a boolean, but it doesn't :(

Of course, kNoCall is false and all others are true, so we can just use it like
it does :)

For now it could be an actual boolean, couldn't it?

[Vyacheslav Egorov (Google), 2012-08-14 12:31:13]

Thanks for the review. Landing.

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_allocator.cc (right):

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.cc:315: SafepointPosition* new_pos = new
SafepointPosition(pos, locs, NULL);
On 2012/08/14 11:18:16, kmillikin wrote:
> Weird that new_pos has a different type than pos.  Suggestion new_pos =>
> safepoint.

Done.

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.cc:322: ASSERT(safepoints_tail_->pos() < pos);
On 2012/08/14 11:18:16, kmillikin wrote:
> Comment that we assume the list is sorted by position when splitting it.

Done.

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_allocator.h (right):

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_allocator.h:422: safepoints_(NULL),
On 2012/08/14 11:18:16, kmillikin wrote:
> I think the insertion code reads better if you change safepoints_ =>
> first_safepoint_ and safepoints_tail_ => last_safepoint_.

Done.

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/locations.h
File runtime/vm/locations.h (right):

https://chromiumcodereview.appspot.com/10823308/diff/1/runtime/vm/locations.h...
runtime/vm/locations.h:298: ContainsCall contains_call() const {
On 2012/08/14 11:18:16, kmillikin wrote:
> It sounds like this returns a boolean, but it doesn't :(
> 
> Of course, kNoCall is false and all others are true, so we can just use it
like
> it does :)
> 
> For now it could be an actual boolean, couldn't it?

Done.

[srdjan, 2012-08-15 00:12:03]

DBC

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/flow_gra...
File runtime/vm/flow_graph_allocator.cc (right):

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/flow_gra...
runtime/vm/flow_graph_allocator.cc:1229: template<typename PositionType>
I think this kind of code sharing makes code less maintainable, and would prefer
to use subclassing instead.

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/flow_gra...
File runtime/vm/flow_graph_allocator.h (right):

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/flow_gra...
runtime/vm/flow_graph_allocator.h:390: class SafepointPosition : public
ZoneAllocated {
Make a common superclass PositionsList (subclasses UsePosition, SafePosition),
and replace the template function for splitting the list

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/flow_gra...
File runtime/vm/flow_graph_compiler.cc (right):

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/flow_gra...
runtime/vm/flow_graph_compiler.cc:213: assembler()->PushRegister(reg);
How does GC work here, e.g., if a register contains an object? Is it only
because all registers currently contain boxed values and are above the
spill-slot limit, i.e., they are automatically considered objects? Please add
comment, thanks.

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/locations.h
File runtime/vm/locations.h (right):

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/location...
runtime/vm/locations.h:298: bool contains_call() const {
I find is_call vs contains_call too similar/confusing

maybe is_always_call vs is_slow_path_call ?

Also, I would expect an accessor named contains_call() to return the field
contains_call_

[Kevin Millikin (Google), 2012-08-15 08:02:15]

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/flow_gra...
File runtime/vm/flow_graph_compiler.cc (right):

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/flow_gra...
runtime/vm/flow_graph_compiler.cc:213: assembler()->PushRegister(reg);
On 2012/08/15 00:12:03, srdjan wrote:
> How does GC work here, e.g., if a register contains an object? Is it only
> because all registers currently contain boxed values and are above the
> spill-slot limit, i.e., they are automatically considered objects? Please add
> comment, thanks.

Exactly.  It works now but needs to be changed when we begin deferring more
code.

I'm working on a change to the stackmaps to count these register values, and
when that lands I can safely look for more code to defer.

[Vyacheslav Egorov (Google), 2012-08-15 13:08:07]

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/flow_gra...
File runtime/vm/flow_graph_allocator.cc (right):

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/flow_gra...
runtime/vm/flow_graph_allocator.cc:1229: template<typename PositionType>
On 2012/08/15 00:12:03, srdjan wrote:
> I think this kind of code sharing makes code less maintainable, and would
prefer
> to use subclassing instead.

I keep in mind that we try to avoid using templates and tried subclassing first.
Unfortunately in this particular case it does not fit well because return type
and head pointer will require additional ugly casts: Derived** is not actually
assignable to Base** for obvious reasons, handling of return value will also
require cast.

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/locations.h
File runtime/vm/locations.h (right):

https://chromiumcodereview.appspot.com/10823308/diff/6001/runtime/vm/location...
runtime/vm/locations.h:298: bool contains_call() const {
On 2012/08/15 00:12:03, srdjan wrote:
> I find is_call vs contains_call too similar/confusing
> 
> maybe is_always_call vs is_slow_path_call ?
> 
> Also, I would expect an accessor named contains_call() to return the field
> contains_call_

I rename contains_call() into can_call(), is_call into always_calls()