Closed
Bug 713915
Opened 12 years ago
Closed 12 years ago
IonMonkey: ARM implement some more NYI's, and fix our stack tracking
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mjrosenb, Unassigned)
Details
Attachments
(1 file, 1 obsolete file)
19.87 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Attachment #584608 -
Flags: review?(nicolas.b.pierron)
Comment 1•12 years ago
|
||
Comment on attachment 584608 [details] [diff] [review] Implement two NYI's and fix an assertion failure. Review of attachment 584608 [details] [diff] [review]: ----------------------------------------------------------------- framePushed_ should only be manipulated by MacroAssemblerARMCompat (rebase your changes on top of latest modifications). Avoid reseting the FramePushed, to avoid future miss-understanding. Introduce a new function callWithJSFrame (see callWithExitFrame) to make the descriptor and to account for all the pushes and pops implied by it (framePushed += sizeof(Frame) - sizeof(void*)). ::: js/src/ion/CodeGenerator.cpp @@ +404,5 @@ > // Throw an InternalError for over-recursion. > > #ifdef JS_CPU_ARM > // XXX: callVM has not yet been implemented for ARM. > + masm.breakpoint(); callVM is merged, I should have removed these lines. ::: js/src/ion/arm/CodeGenerator-arm.cpp @@ +1178,3 @@ > // Remember the size of the frame above this point, in case of bailout. > uint32 stack_size = masm.framePushed() - unused_stack; > uint32 size_descriptor = (stack_size << FRAMETYPE_BITS) | IonFrame_JS; nit: stackSize & sizeDescriptor. @@ +1183,5 @@ > > // Nestle sp up to the argument vector. > if (unused_stack) { > + //masm.ma_add(StackPointer, Imm32(unused_stack), StackPointer); > + masm.freeStack(unused_stack); you don't need the "if" around freeStack because it is already done inside it freeStack function, and the computation of the stack_size can be moved below this line such as you avoid the "- unused_stack" @@ +1212,5 @@ > masm.ma_b(&thunk, Assembler::Above); > > // No argument fixup needed. Call the function normally. > masm.ma_ldr(DTRAddr(objreg, DtrOffImm(offsetof(IonScript, method_))), objreg); > masm.ma_ldr(DTRAddr(objreg, DtrOffImm(IonCode::OffsetOfCode())), objreg); I know this is not part of the modification, but you may want to spend time here to use masm.load32(Address(objreg, IonCode::OffsetOfCode()), objreg); at least to be readable by non-ARM developers. @@ +1219,5 @@ > if (!createSafepoint(call)) > return false; > + // there was one push. > + JS_ASSERT(masm.framePushed() == tmpFramePushed + 4); > + masm.setFramePushed(tmpFramePushed); Ok, It took me time to understand, this shift of 4 correspond to the extra padding contained between the returnAddress and the descriptor. The reset done by setFramePushed is used to remove the padding, then … @@ +1238,4 @@ > } > > // Increment to remove IonFramePrefix; decrement to fill FrameSizeClass. > int prefix_garbage = sizeof(IonJSFrameLayout) - sizeof(void*); … you have an error here. The reason is that this formula already accounts for the padding (frameSize - returnAddress). @@ +1246,2 @@ > else if (restore_diff < 0) > + masm.reserveStack(-restore_diff); framePushed is wrong after these. 2 reasons: 1/ you did not used the "Push" function which are updating the framePushed_ variable when you do a push. 2/ prefix_garbage does not account the modification of made with setFramePushed. You should remove "masm.setFramePushed(tmpFramePushed);" and use masm.Push(tokreg); masm.Push(Imm32(sizeDescriptor)); instead of masm.ma_push(tokreg); masm.push(Imm32(size_descriptor)); ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ +794,5 @@ > } > void > MacroAssemblerARM::ma_pop(Register r) > { > + updateFrame(-4); This is not the purpose of this function. updating the framePushed_ counter is the goal of MacroAssemblerARMCompat::Pop @@ +800,5 @@ > } > void > MacroAssemblerARM::ma_push(Register r) > { > + updateFrame(4); MacroAssemblerARMCompat::Push does that for you. @@ +1047,5 @@ > } > void > MacroAssemblerARM::loadPtr(const Address &address, Register dest) > { > + ma_ldr(Operand(address.base, address.offset), dest); nit: use Operand(address) here. @@ +1059,5 @@ > > void > MacroAssemblerARM::storePtr(Register src, const Address &address) > { > + ma_str(src, Operand(address.base, address.offset)); nit: idem. ::: js/src/ion/arm/MacroAssembler-arm.h @@ +305,5 @@ > void ma_pop(Register r); > void ma_push(Register r); > > + void ma_vpop(VFPRegister r); > + void ma_vpush(VFPRegister r); You may want to ship this function with the related patch. Currently them seems to be unused.
Attachment #584608 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 2•12 years ago
|
||
Attachment #584608 -
Attachment is obsolete: true
Attachment #584816 -
Flags: review?(nicolas.b.pierron)
Comment 3•12 years ago
|
||
Comment on attachment 584816 [details] [diff] [review] addresses nits, also implements another NYI or two Review of attachment 584816 [details] [diff] [review]: ----------------------------------------------------------------- Remove unused functions (3) from this patch. Apply modifications to js::ion::MacroAssemblerX86Shared::callWithExitFrame and remove the #ifdef from callVM. r=me ::: js/src/ion/CodeGenerator.cpp @@ +488,3 @@ > masm.bind(&rejoin); > } > + masm.callIon(objreg); I was expecting something like masm.callWithJSFrame(calleereg); ::: js/src/ion/arm/Assembler-arm.h @@ +184,5 @@ > : kind(k), _code (fr.code()), _isInvalid(false), _isMissing(false) > { > JS_ASSERT(_code == (unsigned)fr.code()); > } > + int size() { return isDouble() ? 8 : 4; } This function is not used. ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ +803,5 @@ > ma_dtr(IsStore, sp,Imm32(-4), r, PreIndex); > } > > +void > +MacroAssemblerARM::ma_vpop(VFPRegister r) These functions are not used. Avoid adding unused functions. @@ +810,5 @@ > + transferFloatReg(r); > + finishFloatTransfer(); > +} > +void > +MacroAssemblerARM::ma_vpush(VFPRegister r) idem. @@ +1024,5 @@ > MacroAssemblerARMCompat::callIon(const Register &callee) > { > + JS_ASSERT((framePushed() & 3) == 0); > + if (framePushed() & 7 == 4) { > + ma_callIonHalfPush(callee); Nice trick. but at the end this function should be private because we don't want to call it without the corresponding Frame. @@ +1519,5 @@ > MacroAssemblerARM::ma_callIon(const Register r) > { > // The stack is presently 8 byte aligned > // We want to decrement sp by 8, and write pc+8 into the new sp > + // when we return from this call, sp will be its present value minus 4. nit: // … the new sp. // When … @@ +1521,5 @@ > // The stack is presently 8 byte aligned > // We want to decrement sp by 8, and write pc+8 into the new sp > + // when we return from this call, sp will be its present value minus 4. > + // update the necessary information > + // updateFrame(4); nit: remove these 2 lines. @@ +1526,5 @@ > as_dtr(IsStore, 32, PreIndex, pc, DTRAddr(sp, DtrOffImm(-8))); > as_blx(r); > } > void > MacroAssemblerARM::ma_callIonNoPush(const Register r) nit: I think you can remove it because it seems to be unused. @@ +1530,5 @@ > MacroAssemblerARM::ma_callIonNoPush(const Register r) > { > + // Since we just write the return address into the stack, which is > + // popped on return, the net effect is removing 4 bytes from the stack > + // updateFrame(-4); nit: remove the last line of the comment. @@ +1538,5 @@ > void > MacroAssemblerARM::ma_callIonHalfPush(const Register r) > { > + // since we just push the pc which gets popped on return, sp is unaffected. > + // don't update framePushed. nit: replace comment by something like: // The stack is unaligned by 4 bytes. // We push the pc to the stack to align the stack before the call, when we // return the pc is poped and the stack is restored to its unaligned state. ::: js/src/ion/shared/CodeGenerator-shared.cpp @@ +303,5 @@ > // returned value, use another LIR instruction. > masm.callWithExitFrame(wrapper); > if (!createSafepoint(ins)) > return false; > +#if defined(JS_CPU_ARM) Why not updating callWithExitFrame on x86 too instead of having this #ifdef ?
Attachment #584816 -
Flags: review?(nicolas.b.pierron)
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•