IA32: Native access to TranscendentalCache for sin/cos.

IA32: Native access to TranscendentalCache for sin/cos.

[Lasse Reichstein, 2010-02-22 10:36:39]

IA32-only optimization for review.

[fschneider, 2010-02-22 17:42:54]

http://codereview.chromium.org/652041/diff/19/1027
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/652041/diff/19/1027#newcode8174
src/ia32/codegen-ia32.cc:8174: __ fld_d(FieldOperand(eax,
HeapNumber::kValueOffset));
Just an idea: Could you optimize the FPU push/pop away in the fast case (input
is a heap number, cache hit)?

http://codereview.chromium.org/652041/diff/19/1027#newcode8192
src/ia32/codegen-ia32.cc:8192: __ and_(Operand(ecx),
Immediate(TranscendentalCache::kCacheSize - 1));
This assumes that kCacheSize is a power of two. I'd move the ASSERT from below
up to here.

http://codereview.chromium.org/652041/diff/19/1027#newcode8197
src/ia32/codegen-ia32.cc:8197:
ASSERT(IsPowerOf2(TranscendentalCache::kCacheSize));
Move this ASSERT to above.

http://codereview.chromium.org/652041/diff/19/1027#newcode8228
src/ia32/codegen-ia32.cc:8228: __ cmp(edx, Operand(ecx, kIntSize));  // NOLINT
Isn't this always half the size of a double (32 bits)? (even on x64)

http://codereview.chromium.org/652041/diff/19/1027#newcode8232
src/ia32/codegen-ia32.cc:8232: __ fstp(0);
Could this pop() of the FPU stack go away? (see my idea comment above)

ebx and edx still contain the original double value, if I understand correct?

http://codereview.chromium.org/652041/diff/19/1027#newcode8271
src/ia32/codegen-ia32.cc:8271: case TranscendentalCache::COS: {
Are there potentially more types of transcendental caches? switch(type_) {...}
does not seem necessary here. Maybe just
 
ASSERT(type_ == TranscendentalCache::SIN || type_ == TranscendentalCache::COS)

so that we don't pay extra in release mode.

http://codereview.chromium.org/652041/diff/19/1027#newcode8320
src/ia32/codegen-ia32.cc:8320: __ fprem();
Is there a reason for not using fprem1()?

http://codereview.chromium.org/652041/diff/19/1029
File src/ia32/disasm-ia32.cc (right):

http://codereview.chromium.org/652041/diff/19/1029#newcode681
src/ia32/disasm-ia32.cc:681: case 0: mnem = "fld_`d"; break;
extra '

[Lasse Reichstein, 2010-02-23 10:18:53]

http://codereview.chromium.org/652041/diff/19/1027
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/652041/diff/19/1027#newcode8174
src/ia32/codegen-ia32.cc:8174: __ fld_d(FieldOperand(eax,
HeapNumber::kValueOffset));
Probably. I would need a flag, or two different paths, to know whether I should
load the double value later. I would also need to remember the pointer to the
heap number (which I can otherwise drop after these loads).
In any case, fld is a very cheap operation (for the CPU, it just onloads it on
the FPU and waits for it to complete there). I don't think it's worth
complicating the code for.

http://codereview.chromium.org/652041/diff/19/1027#newcode8192
src/ia32/codegen-ia32.cc:8192: __ and_(Operand(ecx),
Immediate(TranscendentalCache::kCacheSize - 1));
Well spotted. I moved this line up here but forgot the assert.

http://codereview.chromium.org/652041/diff/19/1027#newcode8228
src/ia32/codegen-ia32.cc:8228: __ cmp(edx, Operand(ecx, kIntSize));  // NOLINT
It should be. The cache element holds two integers, which together should be all
the bits of the double.
The kIntSize here matches the layout test in the DEBUG section above.
The NOLINT should go, in any case. It no longer reads sizeof(int32_t).

http://codereview.chromium.org/652041/diff/19/1027#newcode8232
src/ia32/codegen-ia32.cc:8232: __ fstp(0);
I don't think it's worth it.
In the smi case, I need to convert the smi to a double, so the fldi does double
service by both converting and pushing the value. The fstp operation is very
quick as well (often just 1 cycle on modern chips).

http://codereview.chromium.org/652041/diff/19/1027#newcode8271
src/ia32/codegen-ia32.cc:8271: case TranscendentalCache::COS: {
There are potentially more, some of which won't need the same setup as the 2PI
periodic ones, but there's no need for the generality yet. I'll make it into an
ASSERT and make a comment about future extensions.

http://codereview.chromium.org/652041/diff/19/1027#newcode8320
src/ia32/codegen-ia32.cc:8320: __ fprem();
It is slightly slower on some chips, but it also gives a slightly more precise
result. I guess the tradeof should go in the direction of precission in this
case. I'll change it to fprem1.

http://codereview.chromium.org/652041/diff/19/1029
File src/ia32/disasm-ia32.cc (right):

http://codereview.chromium.org/652041/diff/19/1029#newcode681
src/ia32/disasm-ia32.cc:681: case 0: mnem = "fld_`d"; break;
fixed.