X64: Port inline transcendental cache to X64.

X64: Port inline transcendental cache to X64.

[Lasse Reichstein, 2010-05-03 08:48:03]

Ported.

[Mads Ager (chromium), 2010-05-03 09:14:46]

LGTM

http://codereview.chromium.org/1860001/diff/1/5
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1860001/diff/1/5#newcode7404
src/x64/codegen-x64.cc:7404: // Test that eax is a number.
eax -> rax

Move to after the label declarations?

http://codereview.chromium.org/1860001/diff/1/5#newcode7413
src/x64/codegen-x64.cc:7413: ASSERT_EQ(1, kSmiTagSize);
Does the assert add anything when you are using the SmiToInteger32 macro or
should we just remove it?

http://codereview.chromium.org/1860001/diff/1/5#newcode7427
src/x64/codegen-x64.cc:7427: __ cmpq(rbx, FieldOperand(rax,
HeapObject::kMapOffset));
Can we use CompareRoot here?

http://codereview.chromium.org/1860001/diff/1/5#newcode7443
src/x64/codegen-x64.cc:7443: // or h = (h0 ^ (h0 >> 8) ^ (h0 >> 16) ^ (h0 >>
24)) & cacheSize - 1
add parenthesis (cacheSize - 1)?

http://codereview.chromium.org/1860001/diff/1/5#newcode7469
src/x64/codegen-x64.cc:7469: {  // NOLINT - doesn't like a single brace on a
line.
Will the linter be happy with:

  {  TranscendentalCache::Element ...;
     ...
  }

?

http://codereview.chromium.org/1860001/diff/1/5#newcode7485
src/x64/codegen-x64.cc:7485: __ lea(rcx, Operand(rax, rcx, times_8, 0));\
Remove '\' at end of line.

http://codereview.chromium.org/1860001/diff/1/5#newcode7497
src/x64/codegen-x64.cc:7497: // We are short on registers, so use no_reg as
scratch.
This is a left over comment from ia32, remove?

http://codereview.chromium.org/1860001/diff/1/5#newcode7526
src/x64/codegen-x64.cc:7526: // Only free register is edi.
rdi

http://codereview.chromium.org/1860001/diff/1/5#newcode7540
src/x64/codegen-x64.cc:7540: __ andl(rdi, Immediate(0x7ff));
Should we use a couple of named constants for this?

http://codereview.chromium.org/1860001/diff/1/5#newcode7542
src/x64/codegen-x64.cc:7542: (63 + HeapNumber::kExponentBias);
This will fit on line above?

http://codereview.chromium.org/1860001/diff/1/5#newcode7552
src/x64/codegen-x64.cc:7552: __ movq(rdi, static_cast<uint64_t>(0x7ff8)<<48,
RelocInfo::NONE);
We should use a named constant for the nan representation.

http://codereview.chromium.org/1860001/diff/1/5#newcode7584
src/x64/codegen-x64.cc:7584: __ testl(rax, Immediate(0x400 /* C2 */));
We usually do not put comment in here. This seems like something where we should
have a named constant. Or at least move the comment above the testl line and say
'extract C2 field'.

[Lasse Reichstein, 2010-05-03 10:33:44]

http://codereview.chromium.org/1860001/diff/1/5
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1860001/diff/1/5#newcode7404
src/x64/codegen-x64.cc:7404: // Test that eax is a number.
On 2010/05/03 09:14:46, Mads Ager wrote:
> eax -> rax
> 
> Move to after the label declarations?

Done.

http://codereview.chromium.org/1860001/diff/1/5#newcode7413
src/x64/codegen-x64.cc:7413: ASSERT_EQ(1, kSmiTagSize);
Removed.

http://codereview.chromium.org/1860001/diff/1/5#newcode7427
src/x64/codegen-x64.cc:7427: __ cmpq(rbx, FieldOperand(rax,
HeapObject::kMapOffset));
We can, but in this case I don't want to. Using the root array costs a load and
is somewhat shorter, but I'm afraid the load can't be hoisted very much since we
are just after a conditional jump target, so we will be introducing extra
latency.

http://codereview.chromium.org/1860001/diff/1/5#newcode7443
src/x64/codegen-x64.cc:7443: // or h = (h0 ^ (h0 >> 8) ^ (h0 >> 16) ^ (h0 >>
24)) & cacheSize - 1
On 2010/05/03 09:14:46, Mads Ager wrote:
> add parenthesis (cacheSize - 1)?

Done.

http://codereview.chromium.org/1860001/diff/1/5#newcode7469
src/x64/codegen-x64.cc:7469: {  // NOLINT - doesn't like a single brace on a
line.
It probably will, but I'd hate it instead. I'd rather remove the braces
entirely.

http://codereview.chromium.org/1860001/diff/1/5#newcode7485
src/x64/codegen-x64.cc:7485: __ lea(rcx, Operand(rax, rcx, times_8, 0));\
Whoops.

http://codereview.chromium.org/1860001/diff/1/5#newcode7497
src/x64/codegen-x64.cc:7497: // We are short on registers, so use no_reg as
scratch.
Removed.

http://codereview.chromium.org/1860001/diff/1/5#newcode7526
src/x64/codegen-x64.cc:7526: // Only free register is edi.
fixed.

http://codereview.chromium.org/1860001/diff/1/5#newcode7540
src/x64/codegen-x64.cc:7540: __ andl(rdi, Immediate(0x7ff));
Fixed.

http://codereview.chromium.org/1860001/diff/1/5#newcode7542
src/x64/codegen-x64.cc:7542: (63 + HeapNumber::kExponentBias);
On 2010/05/03 09:14:46, Mads Ager wrote:
> This will fit on line above?

Done.

http://codereview.chromium.org/1860001/diff/1/5#newcode7552
src/x64/codegen-x64.cc:7552: __ movq(rdi, static_cast<uint64_t>(0x7ff8)<<48,
RelocInfo::NONE);
On 2010/05/03 09:14:46, Mads Ager wrote:
> We should use a named constant for the nan representation.

Done.

http://codereview.chromium.org/1860001/diff/1/5#newcode7584
src/x64/codegen-x64.cc:7584: __ testl(rax, Immediate(0x400 /* C2 */));
C2 moved to comment.

http://codereview.chromium.org/1860001/diff/1/5#newcode7594
src/x64/codegen-x64.cc:7594: __ movq(rax, rdi);  // Restore eax (allocated
HeapNumber pointer).
eax->rax.