Closed Bug 1001346 Opened 6 years ago Closed 6 years ago

IonMonkey MIPS: Submit OdinMonkey part of MIPS port.

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: rankov, Assigned: rankov)

References

Details

Attachments

(3 files, 11 obsolete files)

19.35 KB, patch
rankov
: review+
Details | Diff | Splinter Review
37.40 KB, patch
rankov
: review+
Details | Diff | Splinter Review
11.08 KB, patch
rankov
: review+
Details | Diff | Splinter Review
This is follow-up on bug 969375

This bug will track submission of OdinMonkey related patches of the MIPS port.
Assignee: nobody → branislav.rankov
Status: NEW → ASSIGNED
Depends on: 969375
Attached patch OdinMonkey-part1.patch (obsolete) — Splinter Review
Attached patch OdinMonkey-part2.patch (obsolete) — Splinter Review
I can review these patches when you are finished with them.
Comment on attachment 8412594 [details] [diff] [review]
OdinMonkey-part2.patch

Douglas,

There are two calls to cache flush that might be needed on ARM too. Let me know if you think so too.
Flags: needinfo?(dtc-moz)
Comment on attachment 8412594 [details] [diff] [review]
OdinMonkey-part2.patch

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

::: js/src/jit/AsmJSLink.cpp
@@ +40,5 @@
>  
>      module->staticallyLink(cx);
>  
> +#ifdef JS_CODEGEN_MIPS
> +    jit::AutoFlushCache::updateTop(uintptr_t(module->codeBase()), module->offsetOfGlobalData());

It is flushed later in DynamicallyLinkModule->initHeap.  A patch is proposed to rework this, so perhaps just leave this out of this patch.

::: js/src/jit/AsmJSModule.cpp
@@ +317,5 @@
> +            *(void **)(code_ + link.patchAtOffset) = code_ + link.targetOffset;
> +        } else {
> +            InstImm *inst = (InstImm *)(code_ + link.patchAtOffset);
> +            Assembler::updateLuiOriValue(inst, inst->next(), (uint32_t)code_ + link.targetOffset);
> +            AutoFlushCache::updateTop(uintptr_t(inst), 8);

Asm.js code is flushed later when dynamically linking so flushing is not necessary here.

@@ +980,5 @@
>          X86 = 0x1,
>          X64 = 0x2,
>          ARM = 0x3,
> +        MIPS = 0x4,
> +        ARCH_BITS = 3

This might clash with the ARM backend flag bits, see Architecture-arm.cpp.  Just change:

#define HWCAP_USE_HARDFP_ABI (1 << 27)
#define HWCAP_ARMv7 (1 << 28)

@@ +1346,5 @@
>  
>      module->staticallyLink(cx);
>  
> +#ifdef JS_CODEGEN_MIPS
> +    jit::AutoFlushCache::updateTop(uintptr_t(module->codeBase()), module->offsetOfGlobalData());

If this is flushing the executable code then it is not necessary here.  The code will be flushed later when dynamically linking.
Flags: needinfo?(dtc-moz)
Attached patch OdinMonkey-part1.patch (obsolete) — Splinter Review
Attachment #8412593 - Attachment is obsolete: true
Attachment #8418030 - Flags: review?(luke)
Attached patch OdinMonkey-part2.patch (obsolete) — Splinter Review
Attachment #8412594 - Attachment is obsolete: true
Attachment #8418031 - Flags: review?(luke)
Luke, thanks for offering to review the patches.

I have updated code with latest changes regarding stack walking. Also, I have made updates that Douglas suggested.
Comment on attachment 8418030 [details] [diff] [review]
OdinMonkey-part1.patch

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

::: js/src/jit/mips/Assembler-mips.cpp
@@ +36,5 @@
> +        Register destReg;
> +        if (GetIntArgReg(usedArgSlots_, &destReg)) {
> +            current_ = ABIArg(destReg);
> +        } else {
> +            current_ = ABIArg(usedArgSlots_ * sizeof(intptr_t));

Style nit: no { } around single-line then/else.
Attachment #8418030 - Flags: review?(luke) → review+
Comment on attachment 8418031 [details] [diff] [review]
OdinMonkey-part2.patch

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

Overall, looks pretty good.  (Great work on adding a new backend, btw, I know it must be challenging.)  A few nits, requests and questions:

::: js/src/jit/AsmJS.cpp
@@ +1573,5 @@
>                  AsmJSModule::RelativeLink link;
>                  link.patchAtOffset = patchAtOffset;
>                  link.targetOffset = targetOffset;
> +#ifdef JS_CODEGEN_MIPS
> +                link.isTableEntry = false;

I'm a little confused here: on ARM, we only use CodeLabels to label, effectively, raw pointers to be patched (in the switch table).  Is that different on MIPS?

@@ +1582,5 @@
>                  labelOffset = *(uintptr_t *)(module_->codeBase() + patchAtOffset);
> +#else
> +                InstImm *inst = (InstImm *)(module_->codeBase() + patchAtOffset);
> +                labelOffset = Assembler::extractLuiOriValue(inst, inst->next());
> +#endif

Instead of this #ifdef, could you instead introduce some static Assembler::extractLabelOffset(void *); ?

@@ +6087,5 @@
>  // On arm, we need to include an extra word of space at the top of the stack so
>  // we can explicitly store the return address before making the call to C++ or
>  // Ion. On x86/x64, this isn't necessary since the call instruction pushes the
>  // return address.
> +#if defined(JS_CODEGEN_ARM) || defined(JS_CODEGEN_MIPS)

Can you update the above comment to say "On ARM/MIPS, ..."?

@@ +6124,5 @@
>      masm.movePtr(IntArgReg1, GlobalReg);
>      masm.ma_vimm(GenericNaN(), NANReg);
> +#elif defined(JS_CODEGEN_MIPS)
> +    masm.movePtr(IntArgReg1, GlobalReg);
> +    masm.loadConstantDouble(GenericNaN(), NANReg);

Can you change the ARM path to use loadConstantDouble, thereby allowing you to collapse both cases into one
#if defined(JS_CODEGEN_ARM) || defined(JS_CODEGEN_MIPS)
?

::: js/src/jit/AsmJSLink.cpp
@@ +63,5 @@
> +    // On MIPS, we cannot push return address just before call to C++ function.
> +    // MIPS ABI doesn't allow this. Because of this, we need to remember the
> +    // offset of the loaction where return address is stored.
> +    int32_t retAddressOffset = activation->retAddressOffset();
> +    returnAddress_ = *(uint8_t**)(sp_ + retAddressOffset);

I may be missing some of the constraints here, but couldn't MIPS also store the return address using the MaybeRetAddr space in the way that ARM does?  Then there would be a static offset from the exitSP to the return address.

::: js/src/jit/AsmJSModule.cpp
@@ +318,5 @@
> +        } else {
> +            InstImm *inst = (InstImm *)(code_ + link.patchAtOffset);
> +            Assembler::updateLuiOriValue(inst, inst->next(), (uint32_t)code_ + link.targetOffset);
> +        }
> +#endif

Can you add a method:
  bool isRawPointerPatch() const;
so that this code can be written, without #ifdefs as:
  if (link.isRawPointerPatch())
    *(void **)(code_ + ...
  else
    Assembler::patchInstructionImmediate(code_, link.patchAtOffset, (uint32_t)code_ + link.targetOffset);
?  (This new patchInstructionImmediate would be MOZ_CRASH("Unused") on non-MIPS.)

::: js/src/jit/AsmJSModule.h
@@ +371,5 @@
>      struct RelativeLink
>      {
> +#ifdef JS_CODEGEN_MIPS
> +        bool isTableEntry;
> +#endif

I'd like to avoid #ifdefs in AsmJS.cpp as much as possible.  To this end, what if you introduced an enum inside RelativeLink:
  enum Kind { RawPointer, CodeLabel, InstructionImmediate };
and replaced isTableEntry with a field:
  Kind kind_;
and gave RelativeLink an explicit ctor:
  explicit RelativeLink(Kind kind) {
#ifdef JS_CODEGEN_MIPS
      kind_ = kind;
#else
      // On ARM, CodeLabels are only used to label raw pointers, so
      // in all cases on ARM, a RelativePatch means patching a raw pointer.
      JS_ASSERT(kind == CodeLabel || kind == RawPointer);
#endif
  }
With this, you can avoid some #ifdefs in AsmJS.cpp by always passing the correct Kind (even on !MIPS).

@@ +752,5 @@
>          JS_ASSERT(i < pod.numGlobalVars_);
>          return sizeof(void*) +
> +#ifdef JS_CODEGEN_MIPS
> +               // MIPS scratch slot
> +               sizeof(void*) +

I'd really rather avoid sticking the scratch slot here; can't you store this on the stack instead inside the interrupt exit?

::: js/src/jit/AsmJSSignalHandlers.cpp
@@ +398,5 @@
>      return reinterpret_cast<uint8_t**>(&PC_sig(context));
> +#else
> +    JS_STATIC_ASSERT(sizeof(PC_sig(context)) == 2*sizeof(void*));
> +    return reinterpret_cast<uint8_t**>(&PC_sig(context));
> +#endif

Can you take out the #ifdef and just remove the JS_STATIC_ASSERT instead?
Attachment #8418031 - Flags: review?(luke)
(In reply to Luke Wagner [:luke] (on partial PTO) from comment #10)
> Overall, looks pretty good.  (Great work on adding a new backend, btw, I
> know it must be challenging.)  A few nits, requests and questions:

Thanks. It is a challenge and a pleasure :)

> ::: js/src/jit/AsmJS.cpp
> @@ +1573,5 @@
> >                  AsmJSModule::RelativeLink link;
> >                  link.patchAtOffset = patchAtOffset;
> >                  link.targetOffset = targetOffset;
> > +#ifdef JS_CODEGEN_MIPS
> > +                link.isTableEntry = false;
>
> I'm a little confused here: on ARM, we only use CodeLabels to label,
> effectively, raw pointers to be patched (in the switch table).  Is that
> different on MIPS?

On MIPS we use CodeLabels in JitRuntime::generateEnterJIT (like X86) and in switch table. They are not raw pointers. They are encoded in two instructions that load a register.
I can make them be raw pointers if I make a similar hack to what ARM does here:
https://hg.mozilla.org/mozilla-central/file/cf89b5d018f8/js/src/jit/arm/Trampoline-arm.cpp#l213

This would be less efficient in EnterJIT, but it might be better for switch+case. And it would avoid #ifdef here. Do you think this is a good idea?

> @@ +1582,5 @@
> >                  labelOffset = *(uintptr_t *)(module_->codeBase() + patchAtOffset);
> > +#else
> > +                InstImm *inst = (InstImm *)(module_->codeBase() + patchAtOffset);
> > +                labelOffset = Assembler::extractLuiOriValue(inst, inst->next());
> > +#endif
>
> Instead of this #ifdef, could you instead introduce some static
> Assembler::extractLabelOffset(void *); ?

If I do the modification described above, this code would go away. Otherwise, I can move make Assembler::extractLabelOffset().


> ::: js/src/jit/AsmJSLink.cpp
> @@ +63,5 @@
> > +    // On MIPS, we cannot push return address just before call to C++ function.
> > +    // MIPS ABI doesn't allow this. Because of this, we need to remember the
> > +    // offset of the loaction where return address is stored.
> > +    int32_t retAddressOffset = activation->retAddressOffset();
> > +    returnAddress_ = *(uint8_t**)(sp_ + retAddressOffset);
>
> I may be missing some of the constraints here, but couldn't MIPS also store
> the return address using the MaybeRetAddr space in the way that ARM does?
> Then there would be a static offset from the exitSP to the return address.

Problem here is the difference in ABI for exits to Interpreter and exits to Ion. On Ion exits, the offset is 0 (return address is part of ABI). On exits to Interpreter, MIPS ABI reserves space for at least 4 registers on stack. If there are more arguments that 4, then there will be more space taken. I put the return address to the stack before ABI parameters. This is why the offset varies.

This logic will be needed for ARM if you ever call a function that has more than 4 arguments. Look at this comment:

https://hg.mozilla.org/mozilla-central/file/cf89b5d018f8/js/src/jit/arm/MacroAssembler-arm.cpp#l3575


> ::: js/src/jit/AsmJSModule.h
> @@ +371,5 @@
> >      struct RelativeLink
> >      {
> > +#ifdef JS_CODEGEN_MIPS
> > +        bool isTableEntry;
> > +#endif
>
> I'd like to avoid #ifdefs in AsmJS.cpp as much as possible.  To this end,
> what if you introduced an enum inside RelativeLink:
>   enum Kind { RawPointer, CodeLabel, InstructionImmediate };
> and replaced isTableEntry with a field:
>   Kind kind_;
> and gave RelativeLink an explicit ctor:
>   explicit RelativeLink(Kind kind) {
> #ifdef JS_CODEGEN_MIPS
>       kind_ = kind;
> #else
>       // On ARM, CodeLabels are only used to label raw pointers, so
>       // in all cases on ARM, a RelativePatch means patching a raw pointer.
>       JS_ASSERT(kind == CodeLabel || kind == RawPointer);
> #endif
>   }
> With this, you can avoid some #ifdefs in AsmJS.cpp by always passing the
> correct Kind (even on !MIPS).

I will make this modification. It should eliminate most of #ifdef related to isTableEntry.

> @@ +752,5 @@
> >          JS_ASSERT(i < pod.numGlobalVars_);
> >          return sizeof(void*) +
> > +#ifdef JS_CODEGEN_MIPS
> > +               // MIPS scratch slot
> > +               sizeof(void*) +
>
> I'd really rather avoid sticking the scratch slot here; can't you store this
> on the stack instead inside the interrupt exit?

The problem is that MIPS doesn't have a nice way to pop 'pc'. You have to clobber a register for a jump to 32 bit address. I made a hack to restore the scratch register during the jump delay slot. I cannot load from stack because I would also have to update the 'sp'.
Here is the piece of code that does this:

>    // Pop resumePC into PC. Use a slot in Global Data to save the scratch.
>    // Look at globalDataBytes()
>    masm.ma_sw(ScratchRegister, Address(GlobalReg, sizeof(intptr_t)));
>    masm.pop(ScratchRegister);
>    // Use delay slot to restore scratch register.
>    masm.as_jr(ScratchRegister);
>    masm.ma_lw(ScratchRegister, Address(GlobalReg, sizeof(intptr_t)));

I can think of two ways to avoid this:

1. Do the whole work inside the interrupt handler. This might be the most painless solution if there is not too much work to be done.

2. Generate another interrupt to do the jump. This might require some hacks, but should work.


PS: I will do the modifications that I haven't commented on.
Attached patch OdinMonkey-part1.patch (obsolete) — Splinter Review
Carry review from previous patch.

Fixed style nit.
Attachment #8418030 - Attachment is obsolete: true
Attachment #8420097 - Flags: review+
Attached patch OdinMonkey-part1.patch (obsolete) — Splinter Review
Rebased patch because it failed to apply to latest code.

Carry review from previous patch.
Attachment #8420097 - Attachment is obsolete: true
Attachment #8425478 - Flags: review+
Attached patch OdinMonkey-part2.patch (obsolete) — Splinter Review
Attachment #8418031 - Attachment is obsolete: true
Attachment #8425543 - Flags: review?(luke)
I have fixed most of the issues listed. Here is what is left to discuss:

(In reply to Luke Wagner [:luke] from comment #10)
> I'm a little confused here: on ARM, we only use CodeLabels to label,
> effectively, raw pointers to be patched (in the switch table).  Is that
> different on MIPS?

I can make the CodeLabels to be similar to ARM. This might effect performance on MIPS. If you want, I can make a follow-up bug for this.

> @@ +752,5 @@
> >          JS_ASSERT(i < pod.numGlobalVars_);
> >          return sizeof(void*) +
> > +#ifdef JS_CODEGEN_MIPS
> > +               // MIPS scratch slot
> > +               sizeof(void*) +
> 
> I'd really rather avoid sticking the scratch slot here; can't you store this
> on the stack instead inside the interrupt exit?

I suggested some different solutions for this. I can make a follow-up bug to investigate them. If that is OK.
(In reply to Branislav Rankov [:rankov] from comment #11)
Sorry for not replying to some questions here earlier:

> This would be less efficient in EnterJIT, but it might be better for
> switch+case. And it would avoid #ifdef here. Do you think this is a good
> idea?

Well, for asm.js, the generateEnterJIT stub isn't used, so if making CodeLabels point to raw pointers makes switch faster AND makes MIPS more like ARM, I'd do it.

> Problem here is the difference in ABI for exits to Interpreter and exits to
> Ion. On Ion exits, the offset is 0 (return address is part of ABI). On exits
> to Interpreter, MIPS ABI reserves space for at least 4 registers on stack.
> If there are more arguments that 4, then there will be more space taken. I
> put the return address to the stack before ABI parameters. This is why the
> offset varies.
> 
> This logic will be needed for ARM if you ever call a function that has more
> than 4 arguments. Look at this comment:
> 
> https://hg.mozilla.org/mozilla-central/file/cf89b5d018f8/js/src/jit/arm/
> MacroAssembler-arm.cpp#l3575

I don't think we'll need that so could you just do a similar thing to ma_callAndStoreRet that asserts that there aren't any args being passed on the stack?

> The problem is that MIPS doesn't have a nice way to pop 'pc'. You have to
> clobber a register for a jump to 32 bit address. I made a hack to restore
> the scratch register during the jump delay slot. I cannot load from stack
> because I would also have to update the 'sp'.
> Here is the piece of code that does this:

Ahh, I see.  And this post (http://www.linux-mips.org/archives/linux-mips/2009-03/msg00000.html) makes it looks like you can't rely on any signal-safe red zone under sp either, huh?

How about this: you need to clobber a register to hold PC, so you clobber HeapReg and then use the delay slow to reload HeapReg via:
  masm.loadPtr(Address(GlobalReg, m.module().heapOffset()), HeapReg);
?
Comment on attachment 8425543 [details] [diff] [review]
OdinMonkey-part2.patch

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

This looks great, just a few more refinements suggested below:

::: js/src/jit/AsmJS.cpp
@@ +6351,5 @@
>  #endif
> +#if defined(JS_CODEGEN_MIPS)
> +    masm.Push(ra);
> +    // Make sure that arguments buffer is properly aligned.
> +    masm.reserveStack(StackAlignment - sizeof(uintptr_t));

Ah, so this seems to be a general problem that, even if StackDecrementForCall guarantees alignment of sp after the push, the argv Value array can be misaligned by the preceding MaybeRetAddr or StackArgBytes().  It seems like, with MaybeRetAddr = 4, StackArgBytes() = 0, then Values would be misaligned for ARM too; does ARM not required doubles to be 8-byte aligned?

Anyhow, let's fix this in a more general way.  The best way I can think of is to replace the "The stack layout looks like" block below with this block:

  // At the point of the call, the stack layout shall be (sp grows to the left):
  //  | retaddr or stack args | padding | Value argv[] | padding | retaddr | caller stack args |
  // The first padding ensures double-alignment of argv; the second ensures sp is aligned.
  JS_ASSERT(!StackArgBytes(invokeArgTypes) || !MaybeRetAddr);
  unsigned offsetToArgv = AlignByte(StackArgBytes(invokeArgTypes) + MaybeRetAddr, StackAlignment);
  unsigned argvBytes = Max<size_t>(1, exit.sig().args().length()) * sizeof(Value);
  unsigned stackDec = StackDecrementForCall(masm, offsetToArgv + argvBytes + MabyeRetAddr);
  masm.reserveStack(stackDec);

Does that work?

::: js/src/jit/AsmJSModule.cpp
@@ +318,5 @@
> +            *(void **)(code_ + link.patchAtOffset) = code_ + link.targetOffset;
> +        } else {
> +            Assembler::patchInstructionImmediate(code_ + link.patchAtOffset,
> +                                                 PatchedImmPtr(code_ + link.targetOffset));
> +        }

Can you hoist a:
  uint8_t *patchAt = code_ + link.patchAtOffset;
to be reused by the branches of the if/else.  As a bonus, the else should fit one one 100 char line and you can drop the { } around then/else branches.
Attachment #8425543 - Flags: review?(luke)
(In reply to Luke Wagner [:luke] from comment #16)
> Well, for asm.js, the generateEnterJIT stub isn't used, so if making
> CodeLabels point to raw pointers makes switch faster AND makes MIPS more
> like ARM, I'd do it.

I will try this out and see what is the performance impact to Ion code.

> > Problem here is the difference in ABI for exits to Interpreter and exits to
> > Ion. On Ion exits, the offset is 0 (return address is part of ABI). On exits
> > to Interpreter, MIPS ABI reserves space for at least 4 registers on stack.
> > If there are more arguments that 4, then there will be more space taken. I
> > put the return address to the stack before ABI parameters. This is why the
> > offset varies.
> >
> > This logic will be needed for ARM if you ever call a function that has more
> > than 4 arguments. Look at this comment:
> >
> > https://hg.mozilla.org/mozilla-central/file/cf89b5d018f8/js/src/jit/arm/
> > MacroAssembler-arm.cpp#l3575
>
> I don't think we'll need that so could you just do a similar thing to
> ma_callAndStoreRet that asserts that there aren't any args being passed on
> the stack?

MIPS ABI reserves 4 words on the stack even if function has no arguments. Because of this offset to the return address would be 16 in case of exit to Interpreter, and 0 in case of exit to Ion. I need a way to make a difference between these two cases. This is why I added the field to AsmJSActivation.

> How about this: you need to clobber a register to hold PC, so you clobber
> HeapReg and then use the delay slow to reload HeapReg via:
>   masm.loadPtr(Address(GlobalReg, m.module().heapOffset()), HeapReg);
> ?

This looks like a good solution. I will test it. I still need the padding in the global data so that global values are aligned.

(In reply to Luke Wagner [:luke] from comment #17)
> ::: js/src/jit/AsmJS.cpp
> @@ +6351,5 @@
> >  #endif
> > +#if defined(JS_CODEGEN_MIPS)
> > +    masm.Push(ra);
> > +    // Make sure that arguments buffer is properly aligned.
> > +    masm.reserveStack(StackAlignment - sizeof(uintptr_t));
>
> Ah, so this seems to be a general problem that, even if
> StackDecrementForCall guarantees alignment of sp after the push, the argv
> Value array can be misaligned by the preceding MaybeRetAddr or
> StackArgBytes().  It seems like, with MaybeRetAddr = 4, StackArgBytes() = 0,
> then Values would be misaligned for ARM too; does ARM not required doubles
> to be 8-byte aligned?

As far as I know, latest ARM CPUs don't require doubles to be aligned. However, there is some performace penalty. I think it is a minor one.

>
> Anyhow, let's fix this in a more general way.  The best way I can think of
> is to replace the "The stack layout looks like" block below with this block:
>
>   // At the point of the call, the stack layout shall be (sp grows to the
> left):
>   //  | retaddr or stack args | padding | Value argv[] | padding | retaddr |
> caller stack args |
>   // The first padding ensures double-alignment of argv; the second ensures
> sp is aligned.
>   JS_ASSERT(!StackArgBytes(invokeArgTypes) || !MaybeRetAddr);
>   unsigned offsetToArgv = AlignByte(StackArgBytes(invokeArgTypes) +
> MaybeRetAddr, StackAlignment);
>   unsigned argvBytes = Max<size_t>(1, exit.sig().args().length()) *
> sizeof(Value);
>   unsigned stackDec = StackDecrementForCall(masm, offsetToArgv + argvBytes +
> MabyeRetAddr);
>   masm.reserveStack(stackDec);
>
> Does that work?

I like this. It works with one change (when assert is removed). The assert here is wrong because StackArgBytes(invokeArgTypes) is at least 16 on MIPS. See comments above.

> Can you hoist a:
>   uint8_t *patchAt = code_ + link.patchAtOffset;
> to be reused by the branches of the if/else.  As a bonus, the else should
> fit one one 100 char line and you can drop the { } around then/else branches.

I'll do this.
(In reply to Branislav Rankov [:rankov] from comment #18)
> MIPS ABI reserves 4 words on the stack even if function has no arguments.
> Because of this offset to the return address would be 16 in case of exit to
> Interpreter, and 0 in case of exit to Ion. I need a way to make a difference
> between these two cases. This is why I added the field to AsmJSActivation.

Ahh, I see, thanks for explaining.  This MIPS arg reserved space seems the same as the Win64 ShadowStackSpace; can you make ShadowStackSpace = 16 (for regularity; perhaps it might even allow collapsing some code paths somewhere...).

So Ion is really the exception here.  (Also, this exception is destined to be removed; the goal is to eventually put both asm.js and Ion in the same JitActivation, removing AsmJSActivation.)  How about this: in GenerateFFIIonExit, or in 0x1 with StackPointer and store that to exitSP.  Then, in AsmJSFrameIterator, you can branch on this bit to know whether exitSP points to the return address or whether you need to add 4.
(In reply to Branislav Rankov [:rankov] from comment #18)
> (In reply to Luke Wagner [:luke] from comment #16)
> > Well, for asm.js, the generateEnterJIT stub isn't used, so if making
> > CodeLabels point to raw pointers makes switch faster AND makes MIPS more
> > like ARM, I'd do it.
> 
> I will try this out and see what is the performance impact to Ion code.
> 

I tried to do this on MIPS. I would need to use PC relative addressing in all places the CodeLabels are used now (generateEnterJIT, buildFakeExitFrame and emitTableSwitchDispatch). This is what ARM does and it is fast on ARM. On MIPS, accessing the value of PC is expensive and requires a hack. 

In conclusion: It would require a hack in 3 palaces in code, and it would be slower.
(In reply to Branislav Rankov [:rankov] from comment #20)
Ah, fair enough, lets not do that.
Attached patch OdinMonkey-part2.patch (obsolete) — Splinter Review
Attachment #8425543 - Attachment is obsolete: true
Attachment #8430061 - Flags: review?(luke)
Attached patch OdinMonkey-part2.patch (obsolete) — Splinter Review
Sorry. I attached the wrong file previously.
Attachment #8430061 - Attachment is obsolete: true
Attachment #8430061 - Flags: review?(luke)
Attachment #8430082 - Flags: review?(luke)
Comment on attachment 8430082 [details] [diff] [review]
OdinMonkey-part2.patch

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

Beautiful, thanks for being so receptive!

Just in case you hadn't, I'd push to try server or run jit_tests on x86/x64/ARM before landing or marking as checkin-needed since this patches touches the common paths a bit.

::: js/src/jit/AsmJS.cpp
@@ +6365,5 @@
> +    // The first padding ensures double-alignment of argv; the second ensures
> +    // sp is aligned.
> +    unsigned offsetToArgv = AlignBytes(StackArgBytes(invokeArgTypes) + MaybeRetAddr, StackAlignment);
> +    unsigned argvBytes = Max<size_t>(1, exit.sig().args().length()) * sizeof(Value);
> +    unsigned stackDec = StackDecrementForCall(masm, offsetToArgv + argvBytes + MaybeRetAddr);

I think you can remove MaybeRetAddr (since it's already included in offsetToArgv).

@@ +6637,5 @@
>          LoadAsmJSActivationIntoRegister(masm, reg0);
>  
>          // Record sp in the AsmJSActivation for stack-walking.
> +#if defined(JS_CODEGEN_MIPS)
> +        // Add a flag to stored SP to indicate that this is exit to Ion.

Maybe embellish a bit:
  // Add a flag to indicate to AsmJSFrameIterator that we are calling into Ion,
  // since the offset from SP to the return address is different when calling Ion
  // vs. the native ABI.

::: js/src/jit/AsmJSLink.cpp
@@ +59,5 @@
>      // callee to immediately push lr. This means that exitSP points to the
>      // return address.
>      returnAddress_ = *(uint8_t**)sp_;
> +#elif defined(JS_CODEGEN_MIPS)
> +    // On MIPS we have two cases. Exit to C++ will store return addres on

"will store the return address at"

@@ +65,5 @@
> +    // sp + 0. We indicate exits to ion by setting the last bit of stored sp.
> +
> +    if (uint32_t(sp_) & 0x1) {
> +        // This is exit to Ion
> +        sp_ = sp_ - 0x1;

For the purpose of clearing the low bit, i think it would be a bit more readable to write this as:
 sp_ ^= 0x1

::: js/src/jit/AsmJSModule.h
@@ +749,5 @@
>      // The global data section is placed after the executable code (i.e., at
>      // offset codeBytes_) in the module's linear allocation. The global data
>      // are laid out in this order:
>      //   0. a pointer/descriptor for the heap that was linked to the module
> +    //   0.1 On MIPS we need a padding slot to align the data.

Ah, how about this: on *all* platforms, we simply allocate sizeof(uint64_t) for the heap pointer.  This should avoid unaligned access penalties on ARM/x86 and also avoid the separate padding word.  Then you can remove this "0.1." line and change the "0." line to "0. a pointer (padded up to 8 bytes to ensure double-alignment of globals) for the heap that was linked to the module".  ("descriptor" no longer applies.)

@@ +766,5 @@
>          return sizeof(void*) +
> +#ifdef JS_CODEGEN_MIPS
> +               // MIPS padding slot
> +               sizeof(void*) +
> +#endif

So that means you could remove this, and just replace sizeof(void*) with sizeof(uint64_t) above.

@@ +781,5 @@
>          JS_ASSERT(i < pod.numGlobalVars_);
>          return sizeof(void*) +
> +#ifdef JS_CODEGEN_MIPS
> +               // MIPS padding slot
> +               sizeof(void*) +

Same
Attachment #8430082 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #24)

> Beautiful, thanks for being so receptive!

Thanks.

> Just in case you hadn't, I'd push to try server or run jit_tests on
> x86/x64/ARM before landing or marking as checkin-needed since this patches
> touches the common paths a bit.

I tested locally, but I will push this to try also and post the link here.

> For the purpose of clearing the low bit, i think it would be a bit more
> readable to write this as:
>  sp_ ^= 0x1

Since sp_ is a pointer, it will have to be:
        JS_ASSERT(sizeof(uint32_t) == sizeof(uint8_t*));
        sp_ = (uint8_t*)((uint32_t)sp_ ^ 0x1);
(In reply to Branislav Rankov [:rankov] from comment #25)
> Since sp_ is a pointer, it will have to be:
>         JS_ASSERT(sizeof(uint32_t) == sizeof(uint8_t*));
>         sp_ = (uint8_t*)((uint32_t)sp_ ^ 0x1);

Yeah, I guess "sp_ -= 1;" is cleaner than that.  Perhaps you could just add a tiny comment "Clear the low bit"?
You might also want to see bug 860736 which changes how we account for the pushed return address on ARM.  In that bug, the push of lr is not included in masm.framePushed (that is, we masm.push instead of masm.Push).  Instead, ARM has AlignmentAtPrologue = 4 (like x86) and AlignmentMidPrologue is removed.  This simplifies a lot of corner cases and I expect you'd want to do the same for MIPS.  Also, it changes some of the asm.js entry/exit stubs so to use, e.g., to use a common masm.ret() for x86/ARM that pops the stack instead of the current masm.abiret().
(In reply to Luke Wagner [:luke] from comment #27)
> You might also want to see bug 860736 which changes how we account for the
> pushed return address on ARM.  In that bug, the push of lr is not included
> in masm.framePushed (that is, we masm.push instead of masm.Push).  Instead,
> ARM has AlignmentAtPrologue = 4 (like x86) and AlignmentMidPrologue is
> removed.  This simplifies a lot of corner cases and I expect you'd want to
> do the same for MIPS.  Also, it changes some of the asm.js entry/exit stubs
> so to use, e.g., to use a common masm.ret() for x86/ARM that pops the stack
> instead of the current masm.abiret().

Thanks for letting me know about this. I will wait for this bug to land and then update my code.
Depends on: 860736
Rebased patch so that it can be applied.

Carry review from previous patch.
Attachment #8425478 - Attachment is obsolete: true
Attachment #8434118 - Flags: review+
Attached patch OdinMonkey-part2.patch (obsolete) — Splinter Review
Rebased the code after bug 860736 has landed. I think that this might need another review.
Attachment #8430082 - Attachment is obsolete: true
Attachment #8434131 - Flags: review?(luke)
Attached patch OdinMonkey-part3.patch (obsolete) — Splinter Review
This is a MIPS follow-up on changes done in bug 860736.
Attachment #8434133 - Flags: review?(luke)
Here is try link for these patches:
https://tbpl.mozilla.org/?tree=Try&rev=786ae4183028
Comment on attachment 8434131 [details] [diff] [review]
OdinMonkey-part2.patch

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

::: js/src/jit/AsmJSLink.cpp
@@ +70,5 @@
> +        sp_ -= 0x1;
> +        returnAddress_ = *(uint8_t**)sp_;
> +    } else {
> +        // This is exit to C++
> +        returnAddress_ = *(uint8_t**)(sp_ + 4 * sizeof(uintptr_t));

Can you use ShadowStackSpace here and in the comment as well?

::: js/src/jit/mips/Architecture-mips.h
@@ +27,5 @@
>  namespace js {
>  namespace jit {
>  
>  // Shadow stack space is not required on MIPS.
> +static const uint32_t ShadowStackSpace = 4 * sizeof(uintptr_t);

Perhaps you could use ShadowStackSpace in ABIArgGenerator::stackBytesConsumedSoFar?
Attachment #8434131 - Flags: review?(luke) → review+
Comment on attachment 8434133 [details] [diff] [review]
OdinMonkey-part3.patch

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

Ah, so much nicer.

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +3151,5 @@
>      *stackAdjust += usedArgSlots_ > NumIntArgRegs ?
>                      usedArgSlots_ * sizeof(intptr_t) :
>                      NumIntArgRegs * sizeof(intptr_t);
>  
> +    uint32_t alignmentAtPrologue = (callFromAsmJS) ? AlignmentAtAsmJSPrologue : 0;

Can you remove the ( ) around callFromAsmJS (and also in MacroAssembler-arm.cpp; I should have caught this in that review).  Thanks!
Attachment #8434133 - Flags: review?(luke) → review+
Added ShadowStackSpace to mentioned places.

Carry review from previous patch.
Attachment #8434131 - Attachment is obsolete: true
Attachment #8434889 - Flags: review+
Fixed (callFromAsmJS) in MIPS and ARM.

Carry review from previous patch.
Attachment #8434133 - Attachment is obsolete: true
Attachment #8434893 - Flags: review+
You need to log in before you can comment on or make changes to this bug.