http://codereview.chromium.org/6248004/diff/6001/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right):
http://codereview.chromium.org/6248004/diff/6001/src/arm/code-stubs-arm.cc#newcode2893 src/arm/code-stubs-arm.cc:2893: // Uses registers r0 to r4. Expected input is On 2011/01/13 15:14:36, Søren Gjesse wrote:
Please update this comment to include information on the possible
patching, the
structure of the code being patched, and where the patching delta is
found.
Also args_in_registers() is now called HasArgsInRegisters(). I suggest
using
wording like "args in registers" instead of function names in comments
when
possible.
Done. http://codereview.chromium.org/6248004/diff/6001/src/arm/code-stubs-arm.cc#newcode2963 src/arm/code-stubs-arm.cc:2963: __ ldr(scratch, MemOperand(inline_site)); Introduced a new GetRelocatedValueLocation MacroAssembler helper. On 2011/01/13 15:14:36, Søren Gjesse wrote:
This code pattern (the patching of the constant pool entry) is
duplicated three
times. Please refactor this into either the macro assembler or a
helper
generator function on the InstanceofStub.
http://codereview.chromium.org/6248004/diff/6001/src/arm/code-stubs-arm.cc#newcode2991 src/arm/code-stubs-arm.cc:2991: Register scratch3 = map; On 2011/01/13 15:14:36, Søren Gjesse wrote:
Please add
map = no_reg;
as well.
Done. http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm.cc#newcode1628 src/arm/lithium-codegen-arm.cc:1628: // We use Factory::the_hole_value() on purpose to force relocation to be It is. Updated. On 2011/01/13 15:14:36, Søren Gjesse wrote:
Extend comment with "instead of loading from the root array" (I assume
that is
what you mean).
http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm.cc#newcode1633 src/arm/lithium-codegen-arm.cc:1633: // We use Factory::the_hole_value() on purpose to force relocation to be On 2011/01/13 15:14:36, Søren Gjesse wrote:
Ditto.
Done. http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm.cc#newcode1684 src/arm/lithium-codegen-arm.cc:1684: static const int kAdditionalDelta = 4; // 3 instructions + 1 for reloc info. The +1 was referring to the additional instruction used to load the target in Call. (which is relocated here.) You are right about blocking the constant pool here. Changed the comment and blocked constant pool emission. On 2011/01/13 15:14:36, Søren Gjesse wrote:
I think the 4 here is 4 instructions Call always generates two
instructions, and
the reloc info goes in the constant pool later.
Regarding the constant pool I am quite sure that we need to block the
constant
pool for for these 4 instructions. Otherwise we might potentially get
a constant
pool inside this sequence and the calculation of the patch site will
be wrong. http://codereview.chromium.org/6248004/diff/6001/src/arm/lithium-codegen-arm.cc#newcode1688 src/arm/lithium-codegen-arm.cc:1688: __ mov(temp, Operand(delta * kPointerSize)); Added SafepointRegisterSlot and StoreToSafepointRegister helpers. This is not optimal as it only use an offset from sp... On 2011/01/13 15:14:36, Søren Gjesse wrote:
On IA32 we have EspIndexForPushAll, and I think we need something like
that
here, maybe a macro assembler instruction like
StoreToSafePointRegister to work
in correspondance with PushSafepointRegisters/PopSafepointRegisters. mailto:[email protected] is also working on the safepoint stuff
adding saving of
doubles, see http://codereview.chromium.org/6164005/.
http://codereview.chromium.org/6248004/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
