Thanks Sergey, Anton for your comments. Could you please review the updated
patch
http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):
http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1414
src/arm/macro-assembler-arm.cc:1414: mov(scratch,
Operand(unwind_space));
On 2011/01/11 19:27:40, antonm wrote:
Zaheer,
I am not sure it's something which cannot be solved.
I do agree with Sergey and think we should allocate v8::Arguments
below exit
frame (exactly like we do for ia32)---one of the reasons I asked you
to refactor
the code was to make these things immediately visible.
split the code as EnterApiExitFramePrologue/Epilogue and yes, the
arguments is below the exit frame
exit frame
arguments
<stack grows here>
We deal with alignment on x64 as well and it looks fine (in the worst
case you
would need the same and_ trick, see PrepareCallCFunction).
I can reuse PrepareCallCFunction (instead of EnterExitFrameEpilogue) but
it seems to be creating one extra space and storing restore sp which is
not required (i.e. two instructions redundant)
And so on.
On 2011/01/11 15:44:35, zaheer wrote:
> On 2011/01/11 14:11:33, antonm wrote:
> > it looks like a lot of code below is shared with
> MacroAssembler::EnterExitFrame.
> > Is it possible to refactor the common code and make it more like
ia32
> > implementation (which just invokes EnterApiExitFrame + some magic
for some
> > platforms?
> The behavior is different from EnterExitFrame
> - v8::arguments has to be allocated above exit frame to miss gc
> - The alignment has to happen after allocating space for the args
and it
should
> handle aligned/unaligned case based on arg_stack_space (unlike
EnterExitFrame
> which knows the number of pushes - 5)
> - exit frame pc patching has to be handled
> - argv and builtin function is n/a in this case
http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1415
src/arm/macro-assembler-arm.cc:1415: add(ip, sp, Operand(scratch, LSL,
kPointerSizeLog2));
On 2011/01/11 16:34:50, SeRya wrote:
add(ip, sp, Operand(unwind_space * kPointerSize)); ?
Done
http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1432
src/arm/macro-assembler-arm.cc:1432: push(scratch, nz);
On 2011/01/11 16:34:50, SeRya wrote:
ASSERT(frame_alignment == 2 * kPointerSize);
Done
http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1435
src/arm/macro-assembler-arm.cc:1435: mov(ip,
Operand(ExternalReference(Top::k_c_entry_fp_address)));
On 2011/01/11 16:34:50, SeRya wrote:
ia32 code allocates C arguments below c_entry_fp_address. It allows to
not care
that C arguments could be reached by GC. I think this semantic should
be
preserved here as well.
i do allocate the args on top of the exit frame similar to ia32. Do i
miss something
[c args]
[Exit Frame] <- c_entry_fp
[fast api args]
[js args]
Maybe the ordering of code is confusing? i have split it in to
EnterApiExitFramePrologue/Epilogue
By the way, you put argc into the argumets stack space what could be
misinterpreted as an object reference (if argc is odd) and crash GC if
it
happens in the called function.
I did verify in Filecycler that there is no issue with gc invoked from
called function.
Also stack alignment placeholder don't need to be initialized if it's
below
c_entry_fp_address.
removed scratch argument. pushed ip (assumed dummy)
http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1452
src/arm/macro-assembler-arm.cc:1452: mov(ip, Operand(next_address));
On 2011/01/11 16:34:50, SeRya wrote:
x64 implementation use offsets to eliminate 2 of 3 ldr instructions:
static int Offset(ExternalReference ref0, ExternalReference ref1) {
int64_t offset = (ref0.address() - ref1.address());
// Check that fits into int.
ASSERT(static_cast<int>(offset) == offset);
return static_cast<int>(offset);
}
ExternalReference next_address =
ExternalReference::handle_scope_next_address();
const int kNextOffset = 0;
const int kLimitOffset = Offset(
ExternalReference::handle_scope_limit_address(),
next_address);
const int kLevelOffset = Offset(
ExternalReference::handle_scope_level_address(),
next_address);
May be it would work here as well?
Done! but i could remove only the mov not ldr instructions. Also i have
fixed r7 for next address removing two mov's.
http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1476
src/arm/macro-assembler-arm.cc:1476: ldr(r0, MemOperand(r0));
On 2011/01/11 16:34:50, SeRya wrote:
LoadRoot(r0, Heap::kUndefinedValueRootIndex, eq);
ldr(r0, MemOperand(r0), ne);
How about it?
Done! Thanks for the pointer, it simplifies the code very much.
http://codereview.chromium.org/6170001/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
