I already have a patch that puts both tagged and untagged results in the transcendental cache. It didn't seem to make much difference, but I can try it again, or pass it on to you.
On Wed, Mar 2, 2011 at 11:09 AM, <[email protected]> wrote: > 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#newcode3080 > 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#newcode3091 > 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#newcode3165 > 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#newcode3170 > 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#newcode3172 > 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#newcode3186 > 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#newcode3187 > 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#newcode3246 > 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#newcode3256 > 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). > > http://codereview.chromium.org/6591073/ > > -- > v8-dev mailing list > [email protected] > http://groups.google.com/group/v8-dev > -- William Hesse Software Engineer [email protected] Google Denmark ApS Frederiksborggade 20B, 1 sal 1360 København K Denmark CVR nr. 28 86 69 84 If you received this communication by mistake, please don't forward it to anyone else (it may contain confidential or privileged information), please erase all copies of it, including all attachments, and please let the sender know it went to the wrong person. Thanks. -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
