Fast compiler: Load globals variables directly from property cells....

Fast compiler: Load globals variables directly from property cells.

This is a first step towards loading globals directly from property cells instead
of going through a load IC.

This change supports only properties with the DontDelete attribute since
we are only able to bailout into the generic code generated by the secondary 
code generator the beginning of a function. The resulting fast-case code is 
specialized for a specific context. When invoked with a different global object, 
it will always bailout to the secondary code.

When loading a property that does not exist at compile-time or a property
that is deleteable we still generate the generic load IC.


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

[Kevin Millikin (Chromium), 2010-02-03 14:22:20]

I don't think the global object of the caller is the one you want to specialize
for.  I think you should be able to make this change without changing the call
ICs at all.

There is some overlap with my change to add the closure to the CompilationInfo
structure which was done exactly to enable specialization for global
load/stores.

http://codereview.chromium.org/565034/diff/4001/4020
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/565034/diff/4001/4020#newcode64
src/arm/fast-codegen-arm.cc:64: void FastCodeGenerator::EmitGlobalMapCheck() {
Why check the map of the global object and not the identity of the object
itself?

If you need the map check, combine the nearly identical code from
EmitGlobalMapCheck and EmitReceiverMapCheck.  You can pass a flag indicating
whether you know staticallyu that it's a heap object.

http://codereview.chromium.org/565034/diff/4001/4022
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/565034/diff/4001/4022#newcode388
src/arm/ic-arm.cc:388: __ ldr(r0, CodeGenerator::GlobalObject());
This seems weird.  The CallIC miss calls CompileLazy or CompileLazyInLoop with
the function to compile.  The function has a context.  The context has a global
object.  Isn't that the one you want to specialize for?

The global object of the caller seems wrong.

http://codereview.chromium.org/565034/diff/4001/4013
File src/compiler.h (right):

http://codereview.chromium.org/565034/diff/4001/4013#newcode43
src/compiler.h:43: Handle<Object> global_object,
I think you actually want the JSFunction being compiled here.  I have an
outstanding change to add it to the CompilationInfo.

http://codereview.chromium.org/565034/diff/4001/4013#newcode50
src/compiler.h:50: has_global_var_access_(false) {
If you change one, please change the name of all the has_XXX_ functions to keep
them uniform.  has_this_properties_ ==> has_this_property_access_.

http://codereview.chromium.org/565034/diff/4001/4005
File src/data-flow.cc (right):

http://codereview.chromium.org/565034/diff/4001/4005#newcode166
src/data-flow.cc:166: if (var->is_global()) {
var->is_global() && !var->is_this()?

http://codereview.chromium.org/565034/diff/4001/4017
File src/fast-codegen.cc (right):

http://codereview.chromium.org/565034/diff/4001/4017#newcode495
src/fast-codegen.cc:495: Handle<GlobalObject> object =
Handle<GlobalObject>::cast(global_object());
This seems too low level.  Can't you do something like:

LookupResult lookup;
global->Lookup(*expr->name(), &lookup);
if (lookup.GetPropertyDetails().IsDontDelete()) {
...
  Handle<Object> cell(global->GetPropertyCell(&lookup));
...
}

http://codereview.chromium.org/565034/diff/4001/4006
File src/fast-codegen.h (right):

http://codereview.chromium.org/565034/diff/4001/4006#newcode116
src/fast-codegen.h:116: void EmitGlobalVariableLoadFromCell(Handle<Object>
cell);
"FromCell" is implicit in the argument.

http://codereview.chromium.org/565034/diff/4001/4018
File src/handles.h (right):

http://codereview.chromium.org/565034/diff/4001/4018#newcode324
src/handles.h:324: Handle<Object> global_object,
As per my other comments, I think you want to get the global object from the
JSFunction.

http://codereview.chromium.org/565034/diff/4001/4008
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/565034/diff/4001/4008#newcode648
src/ia32/codegen-ia32.h:648: friend class CallIC;
This is a bad code smell.  I don't think the CallIC class should need the code
generator even as a utility.

[fschneider, 2010-02-03 17:00:13]

Addressed comments + Merged with Kevin's change.

http://codereview.chromium.org/565034/diff/4001/4020
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/565034/diff/4001/4020#newcode64
src/arm/fast-codegen-arm.cc:64: void FastCodeGenerator::EmitGlobalMapCheck() {
On 2010/02/03 14:22:20, Kevin Millikin wrote:
> Why check the map of the global object and not the identity of the object
> itself?
> 
> If you need the map check, combine the nearly identical code from
> EmitGlobalMapCheck and EmitReceiverMapCheck.  You can pass a flag indicating
> whether you know staticallyu that it's a heap object.

I'm not sure about the map check vs. identity check. Can there be a map
transition on the global object? In that case I have to check the map, right?

http://codereview.chromium.org/565034/diff/4001/4022
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/565034/diff/4001/4022#newcode388
src/arm/ic-arm.cc:388: __ ldr(r0, CodeGenerator::GlobalObject());
On 2010/02/03 14:22:20, Kevin Millikin wrote:
> This seems weird.  The CallIC miss calls CompileLazy or CompileLazyInLoop with
> the function to compile.  The function has a context.  The context has a
global
> object.  Isn't that the one you want to specialize for?
> 
> The global object of the caller seems wrong.

Done.

http://codereview.chromium.org/565034/diff/4001/4013
File src/compiler.h (right):

http://codereview.chromium.org/565034/diff/4001/4013#newcode43
src/compiler.h:43: Handle<Object> global_object,
On 2010/02/03 14:22:20, Kevin Millikin wrote:
> I think you actually want the JSFunction being compiled here.  I have an
> outstanding change to add it to the CompilationInfo.

Done.

http://codereview.chromium.org/565034/diff/4001/4013#newcode50
src/compiler.h:50: has_global_var_access_(false) {
On 2010/02/03 14:22:20, Kevin Millikin wrote:
> If you change one, please change the name of all the has_XXX_ functions to
keep
> them uniform.  has_this_properties_ ==> has_this_property_access_.

Done.

http://codereview.chromium.org/565034/diff/4001/4005
File src/data-flow.cc (right):

http://codereview.chromium.org/565034/diff/4001/4005#newcode166
src/data-flow.cc:166: if (var->is_global()) {
On 2010/02/03 14:22:20, Kevin Millikin wrote:
> var->is_global() && !var->is_this()?

Done.

http://codereview.chromium.org/565034/diff/4001/4017
File src/fast-codegen.cc (right):

http://codereview.chromium.org/565034/diff/4001/4017#newcode495
src/fast-codegen.cc:495: Handle<GlobalObject> object =
Handle<GlobalObject>::cast(global_object());
On 2010/02/03 14:22:20, Kevin Millikin wrote:
> This seems too low level.  Can't you do something like:
> 
> LookupResult lookup;
> global->Lookup(*expr->name(), &lookup);
> if (lookup.GetPropertyDetails().IsDontDelete()) {
> ...
>   Handle<Object> cell(global->GetPropertyCell(&lookup));
> ...
> }

Good point. I remember trying using Lookup - for some reasone I did it this way.
Let me check again...

http://codereview.chromium.org/565034/diff/4001/4006
File src/fast-codegen.h (right):

http://codereview.chromium.org/565034/diff/4001/4006#newcode116
src/fast-codegen.h:116: void EmitGlobalVariableLoadFromCell(Handle<Object>
cell);
On 2010/02/03 14:22:20, Kevin Millikin wrote:
> "FromCell" is implicit in the argument.

Done.

http://codereview.chromium.org/565034/diff/4001/4018
File src/handles.h (right):

http://codereview.chromium.org/565034/diff/4001/4018#newcode324
src/handles.h:324: Handle<Object> global_object,
On 2010/02/03 14:22:20, Kevin Millikin wrote:
> As per my other comments, I think you want to get the global object from the
> JSFunction.

Done.

http://codereview.chromium.org/565034/diff/4001/4008
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/565034/diff/4001/4008#newcode648
src/ia32/codegen-ia32.h:648: friend class CallIC;
On 2010/02/03 14:22:20, Kevin Millikin wrote:
> This is a bad code smell.  I don't think the CallIC class should need the code
> generator even as a utility.

Done. The IC code is left completely unchanged.

[fschneider, 2010-02-04 10:04:11]

+ Simplified lookup of property cells according to comments.
+ Fixed a bunch of lint errors.

[Kevin Millikin (Chromium), 2010-02-05 09:48:08]

Looks good.

I think you should refactor the explicit EmitXXXMapCheck functions, they won't
scale well.

http://codereview.chromium.org/565034/diff/8001/9008
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/565034/diff/8001/9008#newcode64
src/arm/fast-codegen-arm.cc:64: void FastCodeGenerator::EmitGlobalMapCheck() {
This code is duplicated from EmitReceiverMapCheck.  You should define
EmitMapCheck(Register reg, Label* fail, bool is_heap_object) instead of two
separate map check functions.

http://codereview.chromium.org/565034/diff/8001/9008#newcode82
src/arm/fast-codegen-arm.cc:82: PropertyDetails details) {
I'm not a fan of passing this second argument just for the assert.

http://codereview.chromium.org/565034/diff/8001/9007
File src/fast-codegen.cc (right):

http://codereview.chromium.org/565034/diff/8001/9007#newcode217
src/fast-codegen.cc:217: if (!var->is_global()) BAILOUT("Non-global variable");
I guess this should be !var->is_global() || var->is_this().

For the purposes of simplifying liveness analysis for now, I'm OK with bailing
out for variables we can't handle at compile time.

http://codereview.chromium.org/565034/diff/8001/9004
File src/fast-codegen.h (right):

http://codereview.chromium.org/565034/diff/8001/9004#newcode78
src/fast-codegen.h:78: bool has_receiver() { return info_->has_receiver(); }
Good to push this function into the CompilationInfo.

http://codereview.chromium.org/565034/diff/8001/9004#newcode84
src/fast-codegen.h:84: bool has_global_object() {
This function should be implemented on CompilationInfo instead.

http://codereview.chromium.org/565034/diff/8001/9004#newcode88
src/fast-codegen.h:88: GlobalObject* global_object() {
And this one.

http://codereview.chromium.org/565034/diff/8001/9004#newcode108
src/fast-codegen.h:108: // global object seen at lazy-compile time.
Just say "compile time".  Nothing depends on us using this only for lazy
compilation.

http://codereview.chromium.org/565034/diff/8001/9001
File test/mjsunit/compiler/simple-global-access.js (right):

http://codereview.chromium.org/565034/diff/8001/9001#newcode33
test/mjsunit/compiler/simple-global-access.js:33: function f() { this.x = this.y
= this.z = g1; }
Good.  I think we should also check

function g() { this.x = g1; this.y = g2; this.z = g3 }

[fschneider, 2010-02-05 12:50:57]

New version with comments addressed.

http://codereview.chromium.org/565034/diff/8001/9008
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/565034/diff/8001/9008#newcode64
src/arm/fast-codegen-arm.cc:64: void FastCodeGenerator::EmitGlobalMapCheck() {
On 2010/02/05 09:48:08, Kevin Millikin wrote:
> This code is duplicated from EmitReceiverMapCheck.  You should define
> EmitMapCheck(Register reg, Label* fail, bool is_heap_object) instead of two
> separate map check functions.

Done.

http://codereview.chromium.org/565034/diff/8001/9008#newcode82
src/arm/fast-codegen-arm.cc:82: PropertyDetails details) {
On 2010/02/05 09:48:08, Kevin Millikin wrote:
> I'm not a fan of passing this second argument just for the assert.

Agree. On the other hand we need to generate different code (checking for the
hole value) when we support deletable properties in the future. I'll leave it in
for now.

http://codereview.chromium.org/565034/diff/8001/9007
File src/fast-codegen.cc (right):

http://codereview.chromium.org/565034/diff/8001/9007#newcode217
src/fast-codegen.cc:217: if (!var->is_global()) BAILOUT("Non-global variable");
On 2010/02/05 09:48:08, Kevin Millikin wrote:
> I guess this should be !var->is_global() || var->is_this().
> 
> For the purposes of simplifying liveness analysis for now, I'm OK with bailing
> out for variables we can't handle at compile time.

Done.

http://codereview.chromium.org/565034/diff/8001/9004
File src/fast-codegen.h (right):

http://codereview.chromium.org/565034/diff/8001/9004#newcode78
src/fast-codegen.h:78: bool has_receiver() { return info_->has_receiver(); }
On 2010/02/05 09:48:08, Kevin Millikin wrote:
> Good to push this function into the CompilationInfo.

Done.

http://codereview.chromium.org/565034/diff/8001/9004#newcode84
src/fast-codegen.h:84: bool has_global_object() {
On 2010/02/05 09:48:08, Kevin Millikin wrote:
> This function should be implemented on CompilationInfo instead.

Done.

http://codereview.chromium.org/565034/diff/8001/9004#newcode88
src/fast-codegen.h:88: GlobalObject* global_object() {
On 2010/02/05 09:48:08, Kevin Millikin wrote:
> And this one.

Done.

http://codereview.chromium.org/565034/diff/8001/9004#newcode108
src/fast-codegen.h:108: // global object seen at lazy-compile time.
On 2010/02/05 09:48:08, Kevin Millikin wrote:
> Just say "compile time".  Nothing depends on us using this only for lazy
> compilation.

Done.

http://codereview.chromium.org/565034/diff/8001/9001
File test/mjsunit/compiler/simple-global-access.js (right):

http://codereview.chromium.org/565034/diff/8001/9001#newcode33
test/mjsunit/compiler/simple-global-access.js:33: function f() { this.x = this.y
= this.z = g1; }
On 2010/02/05 09:48:08, Kevin Millikin wrote:
> Good.  I think we should also check
> 
> function g() { this.x = g1; this.y = g2; this.z = g3 }

Done.

[Kevin Millikin (Chromium), 2010-02-05 13:18:25]

Please change info_-> to an accessor info()->.  Otherwise, LGTM.