Unroll more StringDictionary lookup probes both for positive and negative dictionary lookups.

Unroll more StringDictionary lookup probes both for positive and negative dictionary lookups.

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

[Vyacheslav Egorov (Chromium), 2011-05-04 13:54:29]

Mads, can you take a look? 

This is ia32 only, I'll port to other platforms if it looks ok to you.

I tried sharing code between negative and positive lookups but the problem is
that they have different register constrains (negative lookup does not have much
registers available). 

Initially I wanted to kill inlined portions in GenerateDictionaryNegativeLookup
and GenerateStringDictionaryProbes but it does not play well for fast path case
(~20% degradation because of various overhead) so I had to leave it.

On a simple test case with colliding property names this change gives 134x
speedup on "false positive" negative lookup case, and 10x speedup on "false
negative" string dictionary lookup case.

[Mads Ager (chromium), 2011-05-04 15:03:21]

LGTM

http://codereview.chromium.org/6932010/diff/1006/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6932010/diff/1006/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:171: Immediate(Smi::FromInt(name->Hash() +
Accidental edit?

[Vyacheslav Egorov (Chromium), 2011-05-05 11:28:14]

Please take another look. I ported it to arm/x64 and cleaned up ia32 version by
moving dictionary lookup code into a single place.

ARM port is slightly dirty.

[Mads Ager (chromium), 2011-05-05 11:54:55]

LGTM

http://codereview.chromium.org/6932010/diff/6001/src/arm/code-stubs-arm.h
File src/arm/code-stubs-arm.h (right):

http://codereview.chromium.org/6932010/diff/6001/src/arm/code-stubs-arm.h#new...
src/arm/code-stubs-arm.h:623: private:
Could you leave line 623 blank and move this to line 624? :-)

http://codereview.chromium.org/6932010/diff/6001/src/ia32/code-stubs-ia32.h
File src/ia32/code-stubs-ia32.h (right):

http://codereview.chromium.org/6932010/diff/6001/src/ia32/code-stubs-ia32.h#n...
src/ia32/code-stubs-ia32.h:454: private:
Ditto.

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:999: __ Integer32ToSmi(rdx, rbx);
This is fixed in Florian's commit too, right?

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.h
File src/x64/code-stubs-x64.h (right):

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.h#new...
src/x64/code-stubs-x64.h:450: private:
Move.

[Vyacheslav Egorov (Chromium), 2011-05-05 12:30:16]

landing

http://codereview.chromium.org/6932010/diff/6001/src/arm/code-stubs-arm.h
File src/arm/code-stubs-arm.h (right):

http://codereview.chromium.org/6932010/diff/6001/src/arm/code-stubs-arm.h#new...
src/arm/code-stubs-arm.h:623: private:
On 2011/05/05 11:54:56, Mads Ager wrote:
> Could you leave line 623 blank and move this to line 624? :-)

Done.

http://codereview.chromium.org/6932010/diff/6001/src/ia32/code-stubs-ia32.h
File src/ia32/code-stubs-ia32.h (right):

http://codereview.chromium.org/6932010/diff/6001/src/ia32/code-stubs-ia32.h#n...
src/ia32/code-stubs-ia32.h:454: private:
On 2011/05/05 11:54:56, Mads Ager wrote:
> Ditto.

Done.

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:999: __ Integer32ToSmi(rdx, rbx);
On 2011/05/05 11:54:56, Mads Ager wrote:
> This is fixed in Florian's commit too, right?

Yep, this is accidental change.

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.h
File src/x64/code-stubs-x64.h (right):

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.h#new...
src/x64/code-stubs-x64.h:450: private:
On 2011/05/05 11:54:56, Mads Ager wrote:
> Move.

Done.

[Lasse Reichstein, 2011-05-05 14:20:13]

A few, late, X64 comments

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4757: __ and_(index,
Use andl instead of and_, one byte shorter.

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4762: __ lea(index, Operand(index, index, times_2,
0));  // index *= 3.
Use leal instead of lea for 32-bit operations - unless you need it to overflow.

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4771: __ Cmp(entity_name,
masm->isolate()->factory()->undefined_value());
We typically use CompareRoot for this.

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4792: __ testq(r0, r0);
If the result is guaranteed to have its nonzero bits in the lower 32 bits, you
can use testl.

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4822: __ and_(r1, r0);
andl

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4826: __ lea(r1, Operand(r1, r1, times_2, 0));  // r1
= r1 * 3
leal

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4844: __ testq(r0, r0);
Possibly testl.

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4852: //  esp[0 * kPointerSize]: return address.
esp->rsp

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4861: //  result_ is zero if lookup failed, non zero
otherwise.
Document that it's nonzero when used as a double-word register.

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4882: __ and_(scratch, Operand(rsp, 0));
andl

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4886: __ lea(index_, Operand(scratch, scratch,
times_2, 0));  // index *= 3.
leal

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4894: __ Cmp(scratch,
masm->isolate()->factory()->undefined_value());
CompareRoot

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4919: __ movq(scratch, Immediate(0));
__ Move(scratch, 0);  // will use xor

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4925: __ movq(scratch, Immediate(1));
Consider using Move

http://codereview.chromium.org/6932010/diff/6001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4930: __ movq(scratch, Immediate(0));
Use Move.