Optimize instanceof further...

Optimize instanceof further

If the instance of is performed against what is beliwed to be a constant global function inline the instance of check and have the call to the instanceof stub in deferred code. The inlined check will be patched by the instanceof stub when called from deferred code. This is indicated by the lithium instruction LInstanceOfKnownGlobal.

To help the patching the delta from the return address to the patch site is placed just below the return address in the edi slot of the pushad/popad ares. This is safe because the edi register (which is pushed last) is a temporary for the lithium instruction.

As the instanceof stub can call other JavaScript an additional marking for saving all double registers have been added.

Also tweaked the instanceof stub to produce true/false objects instead of 0/1 for the case with deferred code.

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

[Mads Ager (chromium), 2010-12-22 13:38:17]

Does it lint? ;)

I'll perform a real review ASAP.

[Mads Ager (chromium), 2011-01-04 17:25:58]

Hairy stuff. :)

First round of comments. Once these are taken care of I would like to take one
more look.

http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc#newcode4851
src/hydrogen.cc:4851: // residing in new space. If it is the assumption used for
optimization is
Simplify to: If it is we assume that the function will stay the same?

http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc#newcode4862
src/hydrogen.cc:4862: Handle<JSGlobalPropertyCell> cell =
Handle<JSGlobalPropertyCell>::null();
Do you need the cell declaration out here? Can't you move it into the if where
you actually fetch the value?

http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc#newcode4878
src/hydrogen.cc:4878: // If the traget is not null we have found a known global
function that is
traget -> target

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:4977: // This code can patch an call site inlined
cache of the instance of check,
an call site -> a call site

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:4983: // address to the cmp instruction just just
below the return address.
just just -> just

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:4985: //   esp[4] : delta from return address to cmp
instruction
You should add to this comment that call site patching only happens when args
are in registers. Begin the generation function with:

ASSERT(HasArgsInRegisters() || !HasCallSiteInlineCheck())

?

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5041: // Update the global instanceof cache with the
current map and function. The
global -> global or inlined?

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5050: // The constatns in for the code patching are
based on no push instructions
constatns in for -> constants for

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5056: __ add(Operand(scratch), Immediate(2));
Named constant? Also, can't this be part of the operand in the move below?

Can you generate code when running with --debug-code that checks that you are
patching a cmp instruction here? Similarly for the moves that you patch below?

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5084: __ add(Operand(scratch), Immediate(13));
Named constant? Combine with move?

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5103: __ add(Operand(scratch), Immediate(13));
Same here.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5154: 
two blank lines between functions

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5156: 
Two blanks.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5179: "InstanceofStub%s%s",
Isn't there a %s missing here?

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1759: // instance off stub.
off -> of

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1803: 
Remove extra newline?

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1807: // offset to the location where to patch
the map.
to the location of the map check?

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1823: // Put the result value into the eax slot
and restore all registres.
registres -> registers

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (left):

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-ia32.h#old...
src/ia32/lithium-ia32.h:975: 
Accidental edit?

http://codereview.chromium.org/5990005/diff/16001/src/lithium-allocator.h
File src/lithium-allocator.h (right):

http://codereview.chromium.org/5990005/diff/16001/src/lithium-allocator.h#new...
src/lithium-allocator.h:800: // Marks the current instruction as a requiring
saving double registers.
as a -> as

[Søren Thygesen Gjesse, 2011-01-05 09:28:00]

Addressed all the comments. Also changed the call site cache to use a near
label, saving 4 bytes.

http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc#newcode4851
src/hydrogen.cc:4851: // residing in new space. If it is the assumption used for
optimization is
On 2011/01/04 17:25:58, Mads Ager wrote:
> Simplify to: If it is we assume that the function will stay the same?

Done.

http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc#newcode4862
src/hydrogen.cc:4862: Handle<JSGlobalPropertyCell> cell =
Handle<JSGlobalPropertyCell>::null();
On 2011/01/04 17:25:58, Mads Ager wrote:
> Do you need the cell declaration out here? Can't you move it into the if where
> you actually fetch the value?

Don't need the cell at all, can just use lookup.GetValue.

http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc#newcode4878
src/hydrogen.cc:4878: // If the traget is not null we have found a known global
function that is
On 2011/01/04 17:25:58, Mads Ager wrote:
> traget -> target

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:4977: // This code can patch an call site inlined
cache of the instance of check,
On 2011/01/04 17:25:58, Mads Ager wrote:
> an call site -> a call site

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:4983: // address to the cmp instruction just just
below the return address.
On 2011/01/04 17:25:58, Mads Ager wrote:
> just just -> just

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:4985: //   esp[4] : delta from return address to cmp
instruction
On 2011/01/04 17:25:58, Mads Ager wrote:
> You should add to this comment that call site patching only happens when args
> are in registers. Begin the generation function with:
> 
> ASSERT(HasArgsInRegisters() || !HasCallSiteInlineCheck())
> 
> ?

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5041: // Update the global instanceof cache with the
current map and function. The
On 2011/01/04 17:25:58, Mads Ager wrote:
> global -> global or inlined?

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5050: // The constatns in for the code patching are
based on no push instructions
On 2011/01/04 17:25:58, Mads Ager wrote:
> constatns in for -> constants for

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5056: __ add(Operand(scratch), Immediate(2));
On 2011/01/04 17:25:58, Mads Ager wrote:
> Named constant? Also, can't this be part of the operand in the move below?
> 
> Can you generate code when running with --debug-code that checks that you are
> patching a cmp instruction here? Similarly for the moves that you patch below?

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5084: __ add(Operand(scratch), Immediate(13));
On 2011/01/04 17:25:58, Mads Ager wrote:
> Named constant? Combine with move?

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5103: __ add(Operand(scratch), Immediate(13));
On 2011/01/04 17:25:58, Mads Ager wrote:
> Same here.

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5154: 
On 2011/01/04 17:25:58, Mads Ager wrote:
> two blank lines between functions

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5156: 
On 2011/01/04 17:25:58, Mads Ager wrote:
> Two blanks.

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5179: "InstanceofStub%s%s",
On 2011/01/04 17:25:58, Mads Ager wrote:
> Isn't there a %s missing here?

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1759: // instance off stub.
On 2011/01/04 17:25:58, Mads Ager wrote:
> off -> of

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1803: 
On 2011/01/04 17:25:58, Mads Ager wrote:
> Remove extra newline?

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1807: // offset to the location where to patch
the map.
On 2011/01/04 17:25:58, Mads Ager wrote:
> to the location of the map check?

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1823: // Put the result value into the eax slot
and restore all registres.
On 2011/01/04 17:25:58, Mads Ager wrote:
> registres -> registers

Done.

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (left):

http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-ia32.h#old...
src/ia32/lithium-ia32.h:975: 
On 2011/01/04 17:25:58, Mads Ager wrote:
> Accidental edit?

Done.

http://codereview.chromium.org/5990005/diff/16001/src/lithium-allocator.h
File src/lithium-allocator.h (right):

http://codereview.chromium.org/5990005/diff/16001/src/lithium-allocator.h#new...
src/lithium-allocator.h:800: // Marks the current instruction as a requiring
saving double registers.
On 2011/01/04 17:25:58, Mads Ager wrote:
> as a -> as

Done.

[Mads Ager (chromium), 2011-01-05 09:59:26]

LGTM

http://codereview.chromium.org/5990005/diff/29001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/5990005/diff/29001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5009: static const int8_t kMovEaxImmediate =
static_cast<int8_t>(0xb8);
Add the 'Byte' suffix to this one as well?

http://codereview.chromium.org/5990005/diff/29001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/5990005/diff/29001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1811: InstanceofStub stub(flags);
Can you move this declaration up to where the flags are defined to make it clear
what they are used for?

[Søren Thygesen Gjesse, 2011-01-05 11:11:01]

http://codereview.chromium.org/5990005/diff/29001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/5990005/diff/29001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5009: static const int8_t kMovEaxImmediate =
static_cast<int8_t>(0xb8);
On 2011/01/05 09:59:26, Mads Ager wrote:
> Add the 'Byte' suffix to this one as well?

Done.

http://codereview.chromium.org/5990005/diff/29001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/5990005/diff/29001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1811: InstanceofStub stub(flags);
On 2011/01/05 09:59:26, Mads Ager wrote:
> Can you move this declaration up to where the flags are defined to make it
clear
> what they are used for?

Done.