Thanks for the review Søren. Could you have a look at my comment on the PushSafepointRegisters before we upload the patch?
The InstanceofStub does not touch the floating point registers, so do we actually need to save them? Alexandre http://codereview.chromium.org/6248004/diff/15001/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/6248004/diff/15001/src/arm/code-stubs-arm.cc#newcode2895 src/arm/code-stubs-arm.cc:2895: // * object: r0 or at sp+1*kPointerSize . On 2011/01/17 13:18:07, Søren Gjesse wrote:
Spaces around binary operations, and no space before ..
Done. http://codereview.chromium.org/6248004/diff/15001/src/arm/code-stubs-arm.cc#newcode3062 src/arm/code-stubs-arm.cc:3062: __ EnterInternalFrame(); On 2011/01/17 13:18:07, Søren Gjesse wrote:
LeaveInternalFrame after the call.
Done. http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-arm.cc File src/arm/lithium-arm.cc (right): http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-arm.cc#newcode1079 src/arm/lithium-arm.cc:1079: return MarkAsCall(DefineFixed(result, r0), instr); On 2011/01/17 13:18:07, Søren Gjesse wrote:
Now that we have fast straight line code and slow code in deferred
code this
should not use MarkAsCall any more, but AssignEnvironment(AssignPointerMap(DefineFixed(result, r0))). The IA32
version
uses MarkAsSaveDoubles(result) as well, but with the newly added
safepoint with
doubles (see comments in lithium-codegen-arm.cc) that should be used
instead. Done. http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm.cc#newcode1995 src/arm/lithium-codegen-arm.cc:1995: __ PushSafepointRegisters(); Do we really need to? The InstanceofStub doesn't use floating point registers, so it does not seem necessary. On 2011/01/17 13:18:07, Søren Gjesse wrote:
I think we need to use PushSafepointRegistersAndDoubles here. We have
no control
over whether the InstanceofStub will change the content of the double
registers. http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm.cc#newcode2007 src/arm/lithium-codegen-arm.cc:2007: __ BlockConstPoolFor(4); On 2011/01/17 13:18:07, Søren Gjesse wrote:
4 -> kAdditionalDelta
Done. http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm.cc#newcode2013 src/arm/lithium-codegen-arm.cc:2013: RecordSafepointWithRegisters( See previous comment about PushSafepointRegisters. On 2011/01/17 13:18:07, Søren Gjesse wrote:
Use RecordSafepointWithRegistersAndDoubles here.
http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm.cc#newcode2017 src/arm/lithium-codegen-arm.cc:2017: __ str(r0, MemOperand(sp, result.code() * kPointerSize)); Done. Added an ASSERT to check that instr->result() is r0 . On 2011/01/17 13:18:07, Søren Gjesse wrote:
Use StoreToSafepointRegisterSlot here.
http://codereview.chromium.org/6248004/diff/15001/src/arm/lithium-codegen-arm.cc#newcode2019 src/arm/lithium-codegen-arm.cc:2019: __ PopSafepointRegisters(); See previous comment about PushSafepointRegisters. On 2011/01/17 13:18:07, Søren Gjesse wrote:
And use PopSafepointRegistersAndDoubles here.
http://codereview.chromium.org/6248004/diff/15001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6248004/diff/15001/src/arm/macro-assembler-arm.cc#newcode488 src/arm/macro-assembler-arm.cc:488: void MacroAssembler::StoreToSafepointRegisters(Register reg) { On 2011/01/17 13:18:07, Søren Gjesse wrote:
StoreToSafepointRegisters -> StoreToSafepointRegisterSlot
Done. http://codereview.chromium.org/6248004/diff/15001/src/arm/macro-assembler-arm.cc#newcode502 src/arm/macro-assembler-arm.cc:502: return MemOperand(sp, reg.code() * kInstrSize); On 2011/01/17 13:18:07, Søren Gjesse wrote:
reg.code() -> SafepointRegisterStackIndex(reg.code())
Done. http://codereview.chromium.org/6248004/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
