Moving more code to lookup an item from the native cache into code generator.

Moving more code to lookup an item from the native cache into code generator.

To bypass expensive invocation of JS functions from C++ and omit runtime
call overhead for searching the cache, more elaborate deferred code is generated.

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

[antonm, 2010-04-22 14:59:34]

Lasse,

may you have a look?

[Lasse Reichstein, 2010-04-23 07:35:12]

LGTM

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

http://codereview.chromium.org/1695007/diff/1/2#newcode6541
src/ia32/codegen-ia32.cc:6541: 
Could these constants be put into the JSFunctionResultCache class.

http://codereview.chromium.org/1695007/diff/1/2#newcode6545
src/ia32/codegen-ia32.cc:6545: // dst_ keeps the finger key as smi as well.
Could you put a comment here saying what registers/variables holds which values
(and whether they are tagged or not)?

http://codereview.chromium.org/1695007/diff/1/2#newcode6556
src/ia32/codegen-ia32.cc:6556: __ cmp(key_, FieldOperand(cache_,
Add a STATIC_ASSERT(kSmiTagSize == 1 && kSmiTag == 0);
to ensure that using times_half_pointer_size does the right thing (so that if it
ever stops being true, we will know to look here for code that needs changing).

http://codereview.chromium.org/1695007/diff/1/2#newcode6606
src/ia32/codegen-ia32.cc:6606: // slow path this optimization would hardly
matter much.
What do you mean by "slow path".

http://codereview.chromium.org/1695007/diff/1/2#newcode6612
src/ia32/codegen-ia32.cc:6612: __ cmp(ebx, FieldOperand(ecx, kFingerOffset));
Of you have a register for it, you might want to read the finger offset here,
instead of comparing to it here and reading it again two instructions down.

http://codereview.chromium.org/1695007/diff/1/2#newcode6625
src/ia32/codegen-ia32.cc:6625: __ add(FieldOperand(ecx, kCacheSizeOffset),
Immediate(2 << 1));
use kEntrySizeSmi for constant.
Also add the constant in a register and write it back instead of using a
read-modify-write operation.
You seem to have edx free at this point.

[antonm, 2010-04-23 10:22:27]

Lasse, as it looks like you're fine with overall approach, starting to port to
other platforms.

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

http://codereview.chromium.org/1695007/diff/1/2#newcode6541
src/ia32/codegen-ia32.cc:6541: 
On 2010/04/23 07:35:12, Lasse Reichstein wrote:
> Could these constants be put into the JSFunctionResultCache class.

Sure

http://codereview.chromium.org/1695007/diff/1/2#newcode6545
src/ia32/codegen-ia32.cc:6545: // dst_ keeps the finger key as smi as well.
On 2010/04/23 07:35:12, Lasse Reichstein wrote:
> Could you put a comment here saying what registers/variables holds which
values
> (and whether they are tagged or not)?

Sure.

http://codereview.chromium.org/1695007/diff/1/2#newcode6556
src/ia32/codegen-ia32.cc:6556: __ cmp(key_, FieldOperand(cache_,
On 2010/04/23 07:35:12, Lasse Reichstein wrote:
> Add a STATIC_ASSERT(kSmiTagSize == 1 && kSmiTag == 0);
> to ensure that using times_half_pointer_size does the right thing (so that if
it
> ever stops being true, we will know to look here for code that needs
changing).

Done.

http://codereview.chromium.org/1695007/diff/1/2#newcode6606
src/ia32/codegen-ia32.cc:6606: // slow path this optimization would hardly
matter much.
On 2010/04/23 07:35:12, Lasse Reichstein wrote:
> What do you mean by "slow path".

I mean that if we need to call factory function, those additional loads would
hardly matter.  Turning slow path into cache miss.

http://codereview.chromium.org/1695007/diff/1/2#newcode6612
src/ia32/codegen-ia32.cc:6612: __ cmp(ebx, FieldOperand(ecx, kFingerOffset));
On 2010/04/23 07:35:12, Lasse Reichstein wrote:
> Of you have a register for it, you might want to read the finger offset here,
> instead of comparing to it here and reading it again two instructions down.

Done and reworked this logic notably overall.  May you have another look?

http://codereview.chromium.org/1695007/diff/1/2#newcode6625
src/ia32/codegen-ia32.cc:6625: __ add(FieldOperand(ecx, kCacheSizeOffset),
Immediate(2 << 1));
On 2010/04/23 07:35:12, Lasse Reichstein wrote:
> use kEntrySizeSmi for constant.

Sorry, missed that.

> Also add the constant in a register and write it back instead of using a
> read-modify-write operation.
> You seem to have edx free at this point.

Done with lea, is it fine?  Overall, why it's faster?

[antonm, 2010-04-26 19:14:18]

Lasse,

I've ported codgen part to x64.  And I am not sure if it's a good idea to make
similar changes in ARM port.

[Lasse Reichstein, 2010-04-29 08:53:54]

LGTM

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

http://codereview.chromium.org/1695007/diff/1/2#newcode6625
src/ia32/codegen-ia32.cc:6625: __ add(FieldOperand(ecx, kCacheSizeOffset),
Immediate(2 << 1));
It's not lea that is faster (it just saves one instruction over a move and and
add), but avoiding the read entirely.
Reads are the worst-case of memory access. If they fail the L1 cache, they can
be very expensive (not likely here, since we loaded the value recently anyway),
and the following operations typically delay for the value to load (especially
in a read-modify-write operation like add to memory).

http://codereview.chromium.org/1695007/diff/1003/9003
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1695007/diff/1003/9003#newcode4294
src/x64/codegen-x64.cc:4294: STATIC_ASSERT(kSmiTagSize == 1);
Remove asserts (comment is no longer true).

http://codereview.chromium.org/1695007/diff/1003/9003#newcode4301
src/x64/codegen-x64.cc:4301: __ SmiSubConstant(dst_, dst_, kEntrySizeSmi);
Consider converting the smi to a 32-bit integer and do computations on that.
With the current encoding, Smi constants are loaded as 64-bit constants in a
separate instruction, so there is an overhead to save.

http://codereview.chromium.org/1695007/diff/1003/9003#newcode4319
src/x64/codegen-x64.cc:4319: // Consider prefetching into some reg.
Don't just consider it. Do it, and convert it to integer from smi so your
loop-variable doesn't have to be a smi.
You should have plenty of registers to work with on X64.

[antonm, 2010-05-06 16:37:07]

Lasse,

major rework, but now covered by tests.  If you have any other ideas what else I
should test, they'd be most appreciated.

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

http://codereview.chromium.org/1695007/diff/1/2#newcode6625
src/ia32/codegen-ia32.cc:6625: __ add(FieldOperand(ecx, kCacheSizeOffset),
Immediate(2 << 1));
On 2010/04/29 08:53:55, Lasse Reichstein wrote:
> It's not lea that is faster (it just saves one instruction over a move and and
> add), but avoiding the read entirely.
> Reads are the worst-case of memory access. If they fail the L1 cache, they can
> be very expensive (not likely here, since we loaded the value recently
anyway),
> and the following operations typically delay for the value to load (especially
> in a read-modify-write operation like add to memory).

Yes, that's precisely for I think it's already in the cache being read just an
instruction before that I used direct add.

http://codereview.chromium.org/1695007/diff/1003/9003
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1695007/diff/1003/9003#newcode4294
src/x64/codegen-x64.cc:4294: STATIC_ASSERT(kSmiTagSize == 1);
On 2010/04/29 08:53:55, Lasse Reichstein wrote:
> Remove asserts (comment is no longer true).

I assume that Smi are twice bigger than ints in ln. 4367-69.  That's probably a
bad idea, fixing.

http://codereview.chromium.org/1695007/diff/1003/9003#newcode4301
src/x64/codegen-x64.cc:4301: __ SmiSubConstant(dst_, dst_, kEntrySizeSmi);
On 2010/04/29 08:53:55, Lasse Reichstein wrote:
> Consider converting the smi to a 32-bit integer and do computations on that.
> With the current encoding, Smi constants are loaded as 64-bit constants in a
> separate instruction, so there is an overhead to save.

Thanks a lot, nice idea, code reworked.

http://codereview.chromium.org/1695007/diff/1003/9003#newcode4319
src/x64/codegen-x64.cc:4319: // Consider prefetching into some reg.
On 2010/04/29 08:53:55, Lasse Reichstein wrote:
> Don't just consider it. Do it, and convert it to integer from smi so your
> loop-variable doesn't have to be a smi.
> You should have plenty of registers to work with on X64.

Done.

[Lasse Reichstein, 2010-05-07 07:08:42]

http://codereview.chromium.org/1695007/diff/1003/9003
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1695007/diff/1003/9003#newcode4294
src/x64/codegen-x64.cc:4294: STATIC_ASSERT(kSmiTagSize == 1);
Definitly a bad idea, and not true.
The current[1] smi encoding keeps the value in the top 32 bits of the 64-bit
value.

[1] And I hope to be able to change that at some point.

http://codereview.chromium.org/1695007/diff/1003/9003#newcode4368
src/x64/codegen-x64.cc:4368: __ lea(rbx, Operand(rdx,
JSFunctionResultCache::kEntrySize << 1));
This does seem wrong if rdx holds a smi. You need to either convert it to
integer, or convert it to a SmiIndex (which amounts to the same thing with the
current encoding) before using it as index register.

[Lasse Reichstein, 2010-05-07 07:32:24]

LGTM

http://codereview.chromium.org/1695007/diff/22001/23004
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1695007/diff/22001/23004#newcode4446
src/x64/codegen-x64.cc:4446: return FieldOperand(array, index,
times_pointer_size,
How about a comment here that explains the expected content of the registers
(array: FixedArray*, index: int32).
Maybe even change additional_offset to be number of elements (instead of
pre-multiplying by kPointerSize at the call site)?

http://codereview.chromium.org/1695007/diff/22001/23004#newcode4608
src/x64/codegen-x64.cc:4608: __ cmpq(key.reg(), ArrayElement(cache.reg(),
tmp.reg()));
I don't think this makes much difference from using SmiIndex, except for getting
to use ArrayElement. I would prefer to keep SmiIndex, since it would stay
optimal even if the smi representation changes.

[antonm, 2010-05-07 11:34:49]

All done, Lasse, thanks a lot for review.

http://codereview.chromium.org/1695007/diff/22001/23004
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1695007/diff/22001/23004#newcode4446
src/x64/codegen-x64.cc:4446: return FieldOperand(array, index,
times_pointer_size,
On 2010/05/07 07:32:24, Lasse Reichstein wrote:
> How about a comment here that explains the expected content of the registers
> (array: FixedArray*, index: int32).
> Maybe even change additional_offset to be number of elements (instead of
> pre-multiplying by kPointerSize at the call site)?

Done.

http://codereview.chromium.org/1695007/diff/22001/23004#newcode4608
src/x64/codegen-x64.cc:4608: __ cmpq(key.reg(), ArrayElement(cache.reg(),
tmp.reg()));
On 2010/05/07 07:32:24, Lasse Reichstein wrote:
> I don't think this makes much difference from using SmiIndex, except for
getting
> to use ArrayElement. I would prefer to keep SmiIndex, since it would stay
> optimal even if the smi representation changes.

Of course.