Optimize functions needing a local context....

Optimize functions needing a local context.

Allocate the context in the prologue. Two issues had to be solved:
(1) deoptimization needs to handle functions with a local context,
(2) we need a safepoint in the prologue.
(Thanks to Kevin.)


Committed: http://code.google.com/p/v8/source/detail?r=6903

[Mads Ager (chromium), 2011-02-17 15:49:07]

A couple of questions. This is looking good. We need to port it to ARM and to
x64 and then it looks just about ready to go. :-)

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:148: __ CallStub(&stub);
Is there a reason for not using CallCode here which would deal with the
safepoint?

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:150: __ CallRuntime(Runtime::kNewContext, 1);
Ditto, is there a reason for not using CallRuntime here which would deal with
the safepoint?

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:152: safepoints_.DefineSafepoint(masm(),
Safepoint::kSimple, 0,
If you cannot use the function above, could you use the RecordSafepoint function
here?

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:171: // registers, so we have use a third
register to avoid
we have use -> we have to use

[Vyacheslav Egorov (Chromium), 2011-02-18 10:23:49]

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:141: int heap_slots = scope()->num_heap_slots()
- Context::MIN_CONTEXT_SLOTS;
Drive by question: this code is basically shared with full-codegen (except for
safepoint).

Can we avoid code duplication?

[rossberg, 2011-02-21 16:40:42]

I adapted the changes to the other backends. Seems to work correctly, although
we currently don't see much of a performance improvement.

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:141: int heap_slots = scope()->num_heap_slots()
- Context::MIN_CONTEXT_SLOTS;
On 2011/02/18 10:23:49, Vyacheslav Egorov wrote:
> Drive by question: this code is basically shared with full-codegen (except for
> safepoint).
> 
> Can we avoid code duplication?

According to Kevin, there unfortunately is no good place to put this code such
that it can be shared. Nor is there a nice way to factor out the (minor)
differences.

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:148: __ CallStub(&stub);
On 2011/02/17 15:49:07, Mads Ager wrote:
> Is there a reason for not using CallCode here which would deal with the
> safepoint?

Yes: that function needs an explicit LInstruction, which we don't have here.

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:150: __ CallRuntime(Runtime::kNewContext, 1);
On 2011/02/17 15:49:07, Mads Ager wrote:
> Ditto, is there a reason for not using CallRuntime here which would deal with
> the safepoint?

Same here.

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:152: safepoints_.DefineSafepoint(masm(),
Safepoint::kSimple, 0,
On 2011/02/17 15:49:07, Mads Ager wrote:
> If you cannot use the function above, could you use the RecordSafepoint
function
> here?

Same problem, we'd need an LPointerMap to use that function.

http://codereview.chromium.org/6534022/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:171: // registers, so we have use a third
register to avoid
On 2011/02/17 15:49:07, Mads Ager wrote:
> we have use -> we have to use

Done.

[Mads Ager (chromium), 2011-02-22 08:23:42]

Could you upload again Andreas? I don't see the changes for the other backends.

I still find the safepoint definition too raw, but feel free to play with that
after committing the change.

http://codereview.chromium.org/6534022/diff/6001/src/ia32/lithium-codegen-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6534022/diff/6001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:178: safepoints_.DefineSafepoint(masm(),
Safepoint::kSimple, 0,
I still find this too raw. We should hide this away. We can allocate a pointer
map with no pointers and use the normal way.

[rossberg, 2011-02-22 15:00:46]

> Could you upload again Andreas? I don't see the changes for the other
backends.

Oops, sorry, forgot to add the files. Done.

> I still find the safepoint definition too raw, but feel free to play with that
> after committing the change.

Done now (in all backends).

http://codereview.chromium.org/6534022/diff/6001/src/ia32/lithium-codegen-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6534022/diff/6001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:178: safepoints_.DefineSafepoint(masm(),
Safepoint::kSimple, 0,
On 2011/02/22 08:23:42, Mads Ager wrote:
> I still find this too raw. We should hide this away. We can allocate a pointer
> map with no pointers and use the normal way.

Done: factored out into a new variation of the RecordSafepoint method.

[Mads Ager (chromium), 2011-02-22 15:21:10]

LGTM