ARM: Implement untagged input for TranscendentalCacheStub.

ARM: Implement untagged input for TranscendentalCacheStub.

[Søren Thygesen Gjesse, 2011-03-02 10:09:45]

LGTM

I am thinking whether updating the cache with heap number objects in the
non-tagged case defeats the purpose of performing double operations using double
registers and not heap number objects.

Maybe we should experiment with having a transcendental cache which can store
both heap number objects and raw double values, or ignore the cache in the
non-tagged case.

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

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3080: const Register scratch1 = r10;
As discussed offline using r10 here is might be the cause of the crashes you are
currently facing.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3091: CpuFeatures::Scope scope(VFP3);
Duplicate CpuFeatures::Scope.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3165: //__ stop("cache hit");
Debug code.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3170: } // if (CpuFeatures::IsSupported(VFP3))
Two spaces before //.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3172: __ bind(&runtime_call);
Maybe the name of this label should be "calculate", as it does no necessarily
call runtime (normally C-runtime is not counted as runtime).

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3186: __ push(r0);
r0 -> cache_entry

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3187: __ push(lr);
The code for calling the C functions are repeated. Maybe refactor it into
TranscendentalCacheStub::GenerateCallCFunction.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3246: __ mov(scratch0, Operand(Smi::FromInt(2 *
kDoubleSize)));
Shouldn't HeapNumber::kSize be enough? Allocating a heap number has already
failed.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3256: __ AllocateHeapNumber(r6, scratch0, scratch1,
r5, &skip_cache);
Any reason for having the skip cache code before this? Jumping forwards in this
case makes easier to read the code (I guess).

[Karl Klose, 2011-03-02 11:16:41]

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

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3080: const Register scratch1 = r10;
Yes, it is. Used different register.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3091: CpuFeatures::Scope scope(VFP3);
On 2011/03/02 10:09:45, Søren Gjesse wrote:
> Duplicate CpuFeatures::Scope.

Done, removed.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3165: //__ stop("cache hit");
On 2011/03/02 10:09:45, Søren Gjesse wrote:
> Debug code.

Done.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3170: } // if (CpuFeatures::IsSupported(VFP3))
On 2011/03/02 10:09:45, Søren Gjesse wrote:
> Two spaces before //.

Done.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3172: __ bind(&runtime_call);
On 2011/03/02 10:09:45, Søren Gjesse wrote:
> Maybe the name of this label should be "calculate", as it does no necessarily
> call runtime (normally C-runtime is not counted as runtime).

Done.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3186: __ push(r0);
On 2011/03/02 10:09:45, Søren Gjesse wrote:
> r0 -> cache_entry

Done.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3187: __ push(lr);
On 2011/03/02 10:09:45, Søren Gjesse wrote:
> The code for calling the C functions are repeated. Maybe refactor it into
> TranscendentalCacheStub::GenerateCallCFunction.

Done.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3246: __ mov(scratch0, Operand(Smi::FromInt(2 *
kDoubleSize)));
On 2011/03/02 10:09:45, Søren Gjesse wrote:
> Shouldn't HeapNumber::kSize be enough? Allocating a heap number has already
> failed.

Done.

http://codereview.chromium.org/6591073/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3256: __ AllocateHeapNumber(r6, scratch0, scratch1,
r5, &skip_cache);
There is no reason. I changed the order.

[Søren Thygesen Gjesse, 2011-03-04 07:44:18]

LGTM

http://codereview.chromium.org/6591073/diff/1025/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6591073/diff/1025/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:3875: __ mov(scratch0, Operand(4 * kPointerSize));
Why not HeapNumber::kSize instead of 4 * kPointerSize. Shouldn't
HeapNumber::kSize be sufficient to cause a GC?

[Karl Klose, 2011-03-04 08:38:59]

> Why not HeapNumber::kSize instead of 4 * kPointerSize. Shouldn't
> HeapNumber::kSize be sufficient to cause a GC?

kHeapSize should be enough, but it triggers an assert  in
Runtime_AllocateInNewSpace that the allocation size must be aligned to
kPointerSize.