Last Comment Bug 713915 - IonMonkey: ARM implement some more NYI's, and fix our stack tracking
: IonMonkey: ARM implement some more NYI's, and fix our stack tracking
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-28 12:01 PST by Marty Rosenberg [:mjrosenb]
Modified: 2012-01-30 17:19 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement two NYI's and fix an assertion failure. (18.29 KB, patch)
2011-12-28 12:01 PST, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
addresses nits, also implements another NYI or two (19.87 KB, patch)
2011-12-29 12:36 PST, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2011-12-28 12:01:28 PST
Created attachment 584608 [details] [diff] [review]
Implement two NYI's and fix an assertion failure.
Comment 1 Nicolas B. Pierron [:nbp] 2011-12-28 14:24:32 PST
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.
Comment 2 Marty Rosenberg [:mjrosenb] 2011-12-29 12:36:33 PST
Created attachment 584816 [details] [diff] [review]
addresses nits, also implements another NYI or two
Comment 3 Nicolas B. Pierron [:nbp] 2011-12-29 14:44:30 PST
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 ?

Note You need to log in before you can comment on or make changes to this bug.