Issue 661469: Port of changes from r3842 (symbol table probing for two character strings) t...

Issue 661469: Port of changes from r3842 (symbol table probing for two character strings) t... (Closed)

Created:
10 years, 9 months ago by Vyacheslav Egorov (Chromium)

Modified:
9 years, 7 months ago

Reviewers:
Søren Thygesen Gjesse, Mads Ager (chromium)

CC:
v8-dev

Base URL:
http://v8.googlecode.com/svn/branches/bleeding_edge/

Visibility:
Public.

Description

Port of changes from r3842 (symbol table probing for two character strings) to x64 and arm Committed: http://code.google.com/p/v8/source/detail?r=4050

Patch Set 4 : '' #

Created: 10 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+659 lines, -38 lines)			Patch
M	src/arm/codegen-arm.h	View		1 chunk	+30 lines, -0 lines	0 comments	Download
M	src/arm/codegen-arm.cc	View	3	8 chunks	+242 lines, -4 lines	0 comments	Download
M	src/arm/macro-assembler-arm.h	View		1 chunk	+16 lines, -0 lines	0 comments	Download
M	src/arm/macro-assembler-arm.cc	View		2 chunks	+36 lines, -9 lines	0 comments	Download
M	src/ia32/codegen-ia32.cc	View		4 chunks	+5 lines, -11 lines	0 comments	Download
M	src/objects.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	src/x64/codegen-x64.h	View	1 2	1 chunk	+27 lines, -0 lines	0 comments	Download
M	src/x64/codegen-x64.cc	View	1 2 3	5 chunks	+243 lines, -14 lines	0 comments	Download
M	src/x64/macro-assembler-x64.h	View	1 2	1 chunk	+14 lines, -0 lines	0 comments	Download
M	src/x64/macro-assembler-x64.cc	View	1 2	1 chunk	+44 lines, -0 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

Vyacheslav Egorov (Chromium)

BTW, this change doesn't show any 7% speedup of string-unpack-code.js benchmark on my x64 desktop. ...

10 years, 9 months ago (2010-03-03 16:45:24 UTC) #1

Mads Ager (chromium)

LGTM, with a couple of comments. Søren, could you have a look as well since ...

10 years, 9 months ago (2010-03-04 09:02:02 UTC) #2

Søren Thygesen Gjesse

LGTM http://codereview.chromium.org/661469/diff/12/1005 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/661469/diff/12/1005#newcode9003 src/x64/codegen-x64.cc:9003: // r8: instance type of first string the ...

10 years, 9 months ago (2010-03-04 10:20:32 UTC) #3

Vyacheslav Egorov (Chromium)

Here comes ARM version. Unlike x64 version it has some major differences in control flow ...

10 years, 9 months ago (2010-03-05 09:54:01 UTC) #4

Here comes ARM version. Unlike x64 version it has some major differences in
control flow from original ia32 version. 

I also found & fixed problem with unbalanced stack in substring stub inherited
from ia32 version. It's rather weird that nothing was crashing.

http://codereview.chromium.org/661469/diff/12/1005
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/661469/diff/12/1005#newcode9003
src/x64/codegen-x64.cc:9003: // r8: instance type of first string
On 2010/03/04 10:20:33, Søren Gjesse wrote:
> the comment should actually be "map first string if string check was performed
> above"

Done.

http://codereview.chromium.org/661469/diff/12/1005#newcode9004
src/x64/codegen-x64.cc:9004: // r9: instance type of first string
On 2010/03/04 10:20:33, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/661469/diff/12/1005#newcode9315
src/x64/codegen-x64.cc:9315: __ Move(undefined, Factory::undefined_value());
On 2010/03/04 09:02:03, Mads Ager wrote:
> __ LoadRoot(undefined, Heap::kUndefinedValueRootIndex);

Done.

http://codereview.chromium.org/661469/diff/12/1005#newcode9339
src/x64/codegen-x64.cc:9339: ASSERT_EQ(1, SymbolTableShape::kEntrySize);
On 2010/03/04 10:20:33, Søren Gjesse wrote:
> Maybe add
> 
>   static int OffsetOfFirstElement()
> 
> to SymbolTabel and use it both here and in the ia32 version.

I looked into objects.h and found handy HashTable::kElementsStartOffset which I
decided to use here.

ASSERTion has to be left in place because it has nothing to do with first
element offset calculation --- it checks that  iteration is done in correct way
(correct scale in addrmode)

http://codereview.chromium.org/661469/diff/12/1005#newcode9358
src/x64/codegen-x64.cc:9358: // As we are out of registers save the mask on the
stack and use that
On 2010/03/04 09:02:03, Mads Ager wrote:
> Do we have one more register on x64 at the call sites?

As Søren pointed we have kScratchRegister which is not part of calling
convention, but fortunately we do no calls here.

I removed "handmade spilling" and switched to temp = kScratchRegister

http://codereview.chromium.org/661469/diff/12/1005#newcode9361
src/x64/codegen-x64.cc:9361: Register temp = mask;
On 2010/03/04 10:20:33, Søren Gjesse wrote:
> I think you can use kScratchRegister as temp here to avoid pushing mask. This
> asumes that JumpIfInstanceTypeIsNotSequentialAscii does not use
> kScratchRegister.

Done.

http://codereview.chromium.org/661469/diff/12/1006
File src/x64/codegen-x64.h (right):

http://codereview.chromium.org/661469/diff/12/1006#newcode781
src/x64/codegen-x64.h:781: // string is found the code falls through with the
string in register eax.
On 2010/03/04 09:02:03, Mads Ager wrote:
> eax -> rax

Done.

> 
> But really, I think this comment should be changed on ia32 as well.  I guess
the
> string will be left in one of the argument registers (c1, c2?) which does not
> have to be eax/rax.

It ends in scratch3 then it is copied to rax (eax in ia32, r0 in arm)

http://codereview.chromium.org/661469/diff/12/1004
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/661469/diff/12/1004#newcode436
src/x64/macro-assembler-x64.h:436: void
JumpIfBothInstanceTypesIsNotSequentialAscii(
On 2010/03/04 10:20:33, Søren Gjesse wrote:
> Is -> Are (in the ia32 version as well I guess).

Done.

There is no such function in ia32 version. I introduced it here.

Søren Thygesen Gjesse

LGTM http://codereview.chromium.org/661469/diff/3001/2010 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/661469/diff/3001/2010#newcode7273 src/arm/codegen-arm.cc:7273: // If check failed combine both characters into ...

10 years, 9 months ago (2010-03-05 10:25:16 UTC) #5

Mads Ager (chromium)

10 years, 9 months ago (2010-03-05 10:28:50 UTC) #6

Vyacheslav Egorov (Chromium)

10 years, 9 months ago (2010-03-05 13:25:56 UTC) #7

http://codereview.chromium.org/661469/diff/3001/2010
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/661469/diff/3001/2010#newcode7273
src/arm/codegen-arm.cc:7273: // If check failed combine both characters into
signle halfword.
On 2010/03/05 10:28:50, Mads Ager wrote:
> signle -> single

Done.

http://codereview.chromium.org/661469/diff/3001/2010#newcode7273
src/arm/codegen-arm.cc:7273: // If check failed combine both characters into
signle halfword.
On 2010/03/05 10:25:16, Søren Gjesse wrote:
> signle -> single. Please add to the comment why the branch is after the orr.

Done.

http://codereview.chromium.org/661469/diff/3001/2010#newcode7284
src/arm/codegen-arm.cc:7284: // Collect the two characters in a register.
On 2010/03/05 10:25:16, Søren Gjesse wrote:
> Isn't this already done above?

No it is not. It is done above only if check fails. It cannot be done above
unconditionally because both characters  are required to calculate hash.

http://codereview.chromium.org/661469/diff/3001/2010#newcode7304
src/arm/codegen-arm.cc:7304: __ sub(mask, mask, Operand(1));
On 2010/03/05 10:25:16, Søren Gjesse wrote:
> Remove one of the empty lines.

Done.

http://codereview.chromium.org/661469/diff/3001/2010#newcode7305
src/arm/codegen-arm.cc:7305: 
On 2010/03/05 10:28:50, Mads Ager wrote:
> Remove extra empty line?

Done.

http://codereview.chromium.org/661469/diff/3001/2010#newcode7310
src/arm/codegen-arm.cc:7310: Operand(SymbolTable::kElementsStartOffset -
kHeapObjectTag));
On 2010/03/05 10:25:16, Søren Gjesse wrote:
> Please remove two of the empty lines.

Done.

http://codereview.chromium.org/661469/diff/3001/2010#newcode7310
src/arm/codegen-arm.cc:7310: Operand(SymbolTable::kElementsStartOffset -
kHeapObjectTag));
On 2010/03/05 10:28:50, Mads Ager wrote:
> Indentation one space off here?

Done.

http://codereview.chromium.org/661469/diff/3001/2010#newcode7311
src/arm/codegen-arm.cc:7311: 
On 2010/03/05 10:28:50, Mads Ager wrote:
> Remove extra empty lines. 

Done.

http://codereview.chromium.org/661469/diff/3001/2010#newcode7317
src/arm/codegen-arm.cc:7317: // symbol_table: symbol table
On 2010/03/05 10:28:50, Mads Ager wrote:
> symbol_table is not used below, so you should describe symbol_table_element
> here.

Done.

http://codereview.chromium.org/661469/diff/3001/2010#newcode7317
src/arm/codegen-arm.cc:7317: // symbol_table: symbol table
On 2010/03/05 10:25:16, Søren Gjesse wrote:
> This is now first_symbol_table_element and it does not contain the symbol
table.

Done.

http://codereview.chromium.org/661469/diff/3001/2010#newcode7324
src/arm/codegen-arm.cc:7324: Label next_probe[kProbes],
next_probe_pop_mask[kProbes];
On 2010/03/05 10:28:50, Mads Ager wrote:
> I don't think you need the pop_mask variant since you do not push the mask in
> this version.

Done (in x64 version too)

http://codereview.chromium.org/661469/diff/3001/2010#newcode7362
src/arm/codegen-arm.cc:7362: __ ldrh(scratch, FieldMemOperand(candidate,
SeqAsciiString::kHeaderSize));
On 2010/03/05 10:28:50, Mads Ager wrote:
> Loading a half-word puts zeros in the other halfword, right?

So it seems. I could not get a hold on official ARM Instruction Set Reference,
but everything I managed to find indicates that ldrh/ldrb do zero-extension of
loaded halfwords/bytes.

http://codereview.chromium.org/661469/diff/3001/2010#newcode7542
src/arm/codegen-arm.cc:7542: __ strh(r3, MemOperand(r1));
On 2010/03/05 10:28:50, Mads Ager wrote:
> Can you combine the add and the strh here and use a FieldMemOperand?
> 
> strh(r3, FieldMemOperand(r0, SeqAsciiString::kHeaderSize))?

Done.

Expand Messages | Collapse Messages