I think we should put it in in some way, as optimized code using transcendental functions with different input values should not cause allocation of heap numbers.
/Søren On Wed, Mar 2, 2011 at 12:14, William Hesse <[email protected]> wrote: > 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
