X64 Crankshaft: Add untagged version of TranscendentalCacheStub to x64, enabl...

X64 Crankshaft: Add untagged version of TranscendentalCacheStub to x64, enable Cos, Sin, and Log in lithium.

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

[William Hesse, 2011-02-24 17:05:56]

There is still a bug here somewhere.

[Lasse Reichstein, 2011-02-25 06:49:26]

LGTM if bug fixed.

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

http://codereview.chromium.org/6580032/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1535: __ subq(rsp, Immediate(kPointerSize));
Use kDoubleSize instead (id it exists, otherwise introduce it first).

http://codereview.chromium.org/6580032/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1546: __ Move(rbx, Factory::heap_number_map());
Use LoadRoot.

http://codereview.chromium.org/6580032/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1558: __ movq(rdx, rbx);
Use movq(rdx,xmm1) to avoid dependencies.

http://codereview.chromium.org/6580032/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1561: // ST[0] == double value
The untagged case haven't pushed the value on the FPU stack here. It only will
if we miss the cache.

http://codereview.chromium.org/6580032/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1658: // we cause a scavenging GC so that future
allocations will succeed.
Why don't we just allocate the size of a HeapNumber, set its map,  and jump back
to just after the failed allocation?

http://codereview.chromium.org/6580032/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1730: __ fld_d(Operand(rsp, 0));
Can't you just fld_d(FieldOperand(kScratchRegister, ...)) directly?

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

http://codereview.chromium.org/6580032/diff/1/src/x64/codegen-x64.cc#newcode7034
src/x64/codegen-x64.cc:7034: TranscendentalCacheStub::UNTAGGED);
Should these really be UNTAGGED?

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

http://codereview.chromium.org/6580032/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:2891: TranscendentalCacheStub::UNTAGGED);
And should these be UNTAGGED in the full compiler? Doesn't seem right!

[William Hesse, 2011-02-25 12:12:38]

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

http://codereview.chromium.org/6580032/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1535: __ subq(rsp, Immediate(kPointerSize));
On 2011/02/25 06:49:26, Lasse Reichstein wrote:
> Use kDoubleSize instead (id it exists, otherwise introduce it first).

Done.

http://codereview.chromium.org/6580032/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1546: __ Move(rbx, Factory::heap_number_map());
On 2011/02/25 06:49:26, Lasse Reichstein wrote:
> Use LoadRoot.

Done.

http://codereview.chromium.org/6580032/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1558: __ movq(rdx, rbx);
On 2011/02/25 06:49:26, Lasse Reichstein wrote:
> Use movq(rdx,xmm1) to avoid dependencies.

Done.

http://codereview.chromium.org/6580032/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1561: // ST[0] == double value
On 2011/02/25 06:49:26, Lasse Reichstein wrote:
> The untagged case haven't pushed the value on the FPU stack here. It only will
> if we miss the cache.

Added comment to this effect.  All code paths correctly handle this difference
between TAGGED and UNTAGGED case.

http://codereview.chromium.org/6580032/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1658: // we cause a scavenging GC so that future
allocations will succeed.
On 2011/02/25 06:49:26, Lasse Reichstein wrote:
> Why don't we just allocate the size of a HeapNumber, set its map,  and jump
back
> to just after the failed allocation?

Because this was added to the code after it was done.  It really does not add
anything to do this, and it is more complex.  The only thing that is missing in
this case is adding it to the cache, and this is unlikely to be worth it.

http://codereview.chromium.org/6580032/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1730: __ fld_d(Operand(rsp, 0));
On 2011/02/25 06:49:26, Lasse Reichstein wrote:
> Can't you just fld_d(FieldOperand(kScratchRegister, ...)) directly?

Done.

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

http://codereview.chromium.org/6580032/diff/1/src/x64/codegen-x64.cc#newcode7034
src/x64/codegen-x64.cc:7034: TranscendentalCacheStub::UNTAGGED);
On 2011/02/25 06:49:26, Lasse Reichstein wrote:
> Should these really be UNTAGGED?

No.  Fixed.

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

http://codereview.chromium.org/6580032/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:2891: TranscendentalCacheStub::UNTAGGED);
On 2011/02/25 06:49:26, Lasse Reichstein wrote:
> And should these be UNTAGGED in the full compiler? Doesn't seem right!

Nope.  Fixed them.