Last Comment Bug 703565 - IonMonkey: Port exit frames to ARM
: IonMonkey: Port exit frames to ARM
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-11-18 05:32 PST by Marty Rosenberg [:mjrosenb]
Modified: 2011-12-07 17:39 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
minimal changes required to get arm back to a green state. (19.70 KB, patch)
2011-11-18 05:32 PST, Marty Rosenberg [:mjrosenb]
sstangl: review+
Details | Diff | Splinter Review
actually implement the new frame layouts + methods correctly (50.86 KB, patch)
2011-11-18 05:34 PST, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2011-11-18 05:32:20 PST
Created attachment 575431 [details] [diff] [review]
minimal changes required to get arm back to a green state.

ExitFrames were recently added to x86/x64.  Arm should get these as well.
Comment 1 Marty Rosenberg [:mjrosenb] 2011-11-18 05:34:20 PST
Created attachment 575432 [details] [diff] [review]
actually implement the new frame layouts + methods correctly

You may wish to have a look at the previous patch attached to this bug for context.
Comment 2 Sean Stangl [:sstangl] 2011-11-18 10:01:52 PST
Comment on attachment 575431 [details] [diff] [review]
minimal changes required to get arm back to a green state.

Review of attachment 575431 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +44,5 @@
>  #include "ion/IonCompartment.h"
>  #include "ion/IonLinker.h"
>  #include "ion/IonFrames.h"
>  #include "ion/Bailouts.h"
> +#include "ion/arm/Bailouts-arm.h"

ion/arm/Bailouts-arm.h should #include ion/Bailouts.h.
Comment 3 Jacob Bramley [:jbramley] 2011-11-23 04:41:40 PST
Comment on attachment 575432 [details] [diff] [review]
actually implement the new frame layouts + methods correctly

Review of attachment 575432 [details] [diff] [review]:
-----------------------------------------------------------------

== Curly brackets everywhere. == (If there's been a general decision to ignore them, I'll stop complaining about them.)

There's nothing fundamentally wrong. There are a few bits that look broken, and I have a few questions. I'm giving r+ as long as they're easy to fix and explain :-)

::: js/src/ion/arm/Architecture-arm.cpp
@@ +10,5 @@
>  }
> +bool has16DR()
> +{
> +    return true;
> +}

This doesn't seem to be called. Is it relevant to this patch? Also, it will always be true when any version of VFP is present, assuming you meant "has 16 D registers".

::: js/src/ion/arm/Architecture-arm.h
@@ +121,5 @@
>      static const uint32 Allocatable = 13;
>  
>      static const uint32 AllMask = (1 << Total) - 1;
> +    static const uint32 ArgRegMask = (1 << r0) | (1 << r1) | (1 << r2) | (1 << r3);
> +    static const uint32 JSCCallMask = (1 << r0) | (1 << r1) | (1 << r2) | (1 << r3);

The distinction between these two is not clear. From the usage, I suspect that JSCCallMask should include r12 (as another clobberable register), but I'm not certain.

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +994,5 @@
>  
>  void
>  CodeGeneratorARM::linkAbsoluteLabels()
>  {
> +    // arm doesn't have deferred doubles, so this whol thing should be a NOP (right?)

1: I'm not sure what a deferred double is.
2: The method name doesn't say anything about doubles. Perhaps it should be renamed, or perhaps there's something else missing here.
3: s/whol/whole/

::: js/src/ion/arm/IonFrames-arm.h
@@ +90,5 @@
>  typedef IonJSFrameLayout IonFrameData;
>  
>  // This Frame is constructed when JS jited code calls a C function.
>  // NOTE: the C calling convention does not put the return address on the stack,
>  //  so we are pretty sol.

You deleted the code, but not the comment. (The comment doesn't seem to make much sense by itself.)

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +679,5 @@
> +    int neg_bottom = 0x1000 - bottom;
> +    // at this point, both off - bottom and off + neg_bottom will be reasonable-ish
> +    // quantities.
> +    if (off < 0) {
> +        Operand2 sub_off = Imm8(-(off-bottom)); // sub_off = bottom - off

The naming of "Imm8" is rather confusing in real usage. I didn't notice before. (Probably not worth for this patch anyway.)

@@ +682,5 @@
> +    if (off < 0) {
> +        Operand2 sub_off = Imm8(-(off-bottom)); // sub_off = bottom - off
> +        if (!sub_off.invalid) {
> +            as_sub(ScratchRegister, base, sub_off, NoSetCond, cc); // - sub_off = off - bottom
> +            as_dtr(ls, 32, Offset, rt, DTRAddr(base, DtrOffImm(bottom)), cc);

This should surely use 'ScratchRegister' rather than 'base'. The same applies to the other variants.

@@ +692,5 @@
> +            as_dtr(ls, 32, Offset, rt, DTRAddr(base, DtrOffImm(-neg_bottom)), cc);
> +            return;
> +        }
> +    } else {
> +        Operand2 sub_off = Imm8(off-bottom); // sub_off = bottom - off

The comment is wrong.

@@ +698,5 @@
> +            as_add(ScratchRegister, base, sub_off, NoSetCond, cc); // - sub_off = off - bottom
> +            as_dtr(ls, 32, Offset, rt, DTRAddr(base, DtrOffImm(bottom)), cc);
> +            return;
> +        }
> +        sub_off = Imm8(off+neg_bottom);// sub_off = -neg_bottom - off

The comment is wrong.

@@ +1190,5 @@
> +}
> +void
> +MacroAssemblerARMCompat::pushValue(ValueOperand val) {
> +    ma_push(val.typeReg());
> +    ma_push(val.payloadReg());

Ideally, these should be combined into a single instruction. Don't you have a nifty API for that? It can be addressed later, anyway.

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +157,5 @@
>      masm.finishDataTransfer();
>      // Throw our return address onto the stack.  this setup seems less-than-ideal
>      aasm->as_dtr(IsStore, 32, Offset, pc, DTRAddr(sp, DtrOffImm(0)));
>      // Call the function.  using lr as the link register would be *so* nice
>      aasm->as_bx(r0);

Use blx anyway, even if the LR value is ignored. If you don't, return stack predicition won't work.

@@ +158,5 @@
>      // Throw our return address onto the stack.  this setup seems less-than-ideal
>      aasm->as_dtr(IsStore, 32, Offset, pc, DTRAddr(sp, DtrOffImm(0)));
>      // Call the function.  using lr as the link register would be *so* nice
>      aasm->as_bx(r0);
>      // The top of the stack now points to *ABOVE* our return address... great

You know that's not safe, don't you?

@@ +161,5 @@
>      aasm->as_bx(r0);
>      // The top of the stack now points to *ABOVE* our return address... great
>      // Load off of the stack the size of our local stack
> +    aasm->as_dtr(IsLoad, 32, Offset, r5, DTRAddr(sp, DtrOffImm(4)));
> +    // TODO: these can be fused into one! I don't think this is true since I added in the lsr.

You can LSR in a memory operand:

LDR r5, [sp, r5, LSR #FRAMETYPE_BITS]!

What you can't do is simultaneously add the offset (of 4). That is, you can do reg+reg(+shift) or reg+imm, but not reg+reg(+shift)+imm, so you still can't fuse these.

@@ +286,5 @@
> +    // [IonFrame]
> +    // bailoutFrame.registersnapshot
> +    // bailoutFrame.fpsnapshot
> +    // bailoutFrame.snapshotOffset
> +    // bailoutFrame.frameSize

Handy comment, thanks.

@@ +394,2 @@
>      // These should be NOPs
> +    masm.setABIArg(0, sp);

That is not a NOP, unless I misunderstand setABIArg. (Is the comment out of date?)

@@ +485,5 @@
> +    }
> +
> +    Register temp = regs.takeAny();
> +    // TODO: investigate if this can be changed into setupAlignedABICall.
> +    masm.setupUnalignedABICall(f.argc(), temp);

setupUnalignedABICall has JS_NOT_REACHED at the moment. Is there another patch underneath this one which implements it?

@@ +528,5 @@
> +    //masm.ma_ldr(Operand(sp, IonExitFrameLayout::offsetOfReturnAddress()), temp);
> +    // ARM's ABI does not ask you to put the return address on the stack, so we don't
> +    // We still need to have the return address, so we use the pc to compute it.
> +    // Presently, I'm hard coding the value because I know exactly what code is being generated
> +    // but I'll set up a as_mov(Register, Label *) that will automatically do this.

Yes, that's important. Hard-coding this is very dangerous (but Ok for now). Perhaps add a TODO so this doens't get forgotten.

Assumptions made about code that is generated tend to be the underlying cause of many bugs in JM. (Second only to literal pools, though arguably that's just a special case of the same thing.) Because of this, the masm interface in JM isn't really an abstraction any more, but rather a series of shortcuts. Changing things even slightly can make things explode, so optimizations are hard to implement and it's all very delicate.
Comment 4 Marty Rosenberg [:mjrosenb] 2011-11-30 22:01:40 PST
(In reply to Jacob Bramley [:jbramley] from comment #3)
> Comment on attachment 575432 [details] [diff] [review] [diff] [details] [review]
> actually implement the new frame layouts + methods correctly
> 
> Review of attachment 575432 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> == Curly brackets everywhere. == (If there's been a general decision to
> ignore them, I'll stop complaining about them.)
> 
Evidently, the JS guide says that blocks that are a single line don't get curly braces, unless it is part of a group where one requires curly braces.
> There's nothing fundamentally wrong. There are a few bits that look broken,
> and I have a few questions. I'm giving r+ as long as they're easy to fix and
> explain :-)
> 
> ::: js/src/ion/arm/Architecture-arm.cpp
> @@ +10,5 @@
> >  }
> > +bool has16DR()
> > +{
> > +    return true;
> > +}
> 
> This doesn't seem to be called. Is it relevant to this patch? Also, it will
> always be true when any version of VFP is present, assuming you meant "has
> 16 D registers".
> 
right you are.  
> ::: js/src/ion/arm/Architecture-arm.h
> @@ +121,5 @@
> >      static const uint32 Allocatable = 13;
> >  
> >      static const uint32 AllMask = (1 << Total) - 1;
> > +    static const uint32 ArgRegMask = (1 << r0) | (1 << r1) | (1 << r2) | (1 << r3);
> > +    static const uint32 JSCCallMask = (1 << r0) | (1 << r1) | (1 << r2) | (1 << r3);
> 
> The distinction between these two is not clear. From the usage, I suspect
> that JSCCallMask should include r12 (as another clobberable register), but
> I'm not certain.
> 
It should, however, both r12 and r14 are not allocatable, so that would only confuse matters.
> ::: js/src/ion/arm/CodeGenerator-arm.cpp
> @@ +994,5 @@
> >  
> >  void
> >  CodeGeneratorARM::linkAbsoluteLabels()
> >  {
> > +    // arm doesn't have deferred doubles, so this whol thing should be a NOP (right?)
> 
> 1: I'm not sure what a deferred double is.
A deferred double is a pool for x86, where all of the doubles that a function uses get placed after the function in memory. However, it is not pool like in that all of the elements are referred to by their absolute address rather than a pc relative offset.
> 2: The method name doesn't say anything about doubles. Perhaps it should be
> renamed, or perhaps there's something else missing here.
Since the doubles are referred to by their absolute address, fixing up the references is "linking absolute labels".
> ::: js/src/ion/arm/MacroAssembler-arm.cpp
> @@ +679,5 @@
> > +    int neg_bottom = 0x1000 - bottom;
> > +    // at this point, both off - bottom and off + neg_bottom will be reasonable-ish
> > +    // quantities.
> > +    if (off < 0) {
> > +        Operand2 sub_off = Imm8(-(off-bottom)); // sub_off = bottom - off
> 
> The naming of "Imm8" is rather confusing in real usage. I didn't notice
> before. (Probably not worth for this patch anyway.)
> 
Yeah.  I called it Imm8m elsewhere.  You have any other suggestions?
> Ideally, these should be combined into a single instruction. Don't you have
> a nifty API for that? It can be addressed later, anyway.
> 
yes, but it'll choke if the order is wrong (and as far as I can tell, with lsra, it is *always* wrong)

> 
> @@ +158,5 @@
> >      // Throw our return address onto the stack.  this setup seems less-than-ideal
> >      aasm->as_dtr(IsStore, 32, Offset, pc, DTRAddr(sp, DtrOffImm(0)));
> >      // Call the function.  using lr as the link register would be *so* nice
> >      aasm->as_bx(r0);
> >      // The top of the stack now points to *ABOVE* our return address... great
> 
> You know that's not safe, don't you?
It is totally safe.  It is merely stating that the return address gets popped, so the stack pointer is 4 higher than it was when we called the function.  The comment has been clarified.


> @@ +394,2 @@
> >      // These should be NOPs
> > +    masm.setABIArg(0, sp);
> 
> That is not a NOP, unless I misunderstand setABIArg. (Is the comment out of
> date?)
> 
It is out of date.
> @@ +485,5 @@
> > +    }
> > +
> > +    Register temp = regs.takeAny();
> > +    // TODO: investigate if this can be changed into setupAlignedABICall.
> > +    masm.setupUnalignedABICall(f.argc(), temp);
> 
> setupUnalignedABICall has JS_NOT_REACHED at the moment. Is there another
> patch underneath this one which implements it?
> 
Not that I remember :-o
I've now implimented it, it can't be any more wrong.

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