1001346 - IonMonkey MIPS: Submit OdinMonkey part of MIPS port.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Branislav Rankov [:rankov]]

This is follow-up on bug 969375

This bug will track submission of OdinMonkey related patches of the MIPS port.

Comment 1

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part1.patch

Comment 2

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part2.patch

Comment 3

[Luke Wagner [:luke]]

I can review these patches when you are finished with them.

Comment 4

[Branislav Rankov [:rankov]]

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.

Comment 5

[Douglas Crosher [:dougc]]

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.

Comment 6

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part1.patch

Comment 7

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part2.patch

Comment 8

[Branislav Rankov [:rankov]]

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 9

[Luke Wagner [:luke]]

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.

Comment 10

[Luke Wagner [:luke]]

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?

Comment 11

[Branislav Rankov [:rankov]]

(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.

Comment 12

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part1.patch

Carry review from previous patch.

Fixed style nit.

Comment 13

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part1.patch

Rebased patch because it failed to apply to latest code.

Carry review from previous patch.

Comment 14

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part2.patch

Comment 15

[Branislav Rankov [:rankov]]

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.

Comment 16

[Luke Wagner [:luke]]

(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 17

[Luke Wagner [:luke]]

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.

Comment 18

[Branislav Rankov [:rankov]]

(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.

Comment 19

[Luke Wagner [:luke]]

(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.

Comment 20

[Branislav Rankov [:rankov]]

(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.

Comment 21

[Luke Wagner [:luke]]

(In reply to Branislav Rankov [:rankov] from comment #20)
Ah, fair enough, lets not do that.

Comment 22

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part2.patch

Comment 23

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part2.patch

Sorry. I attached the wrong file previously.

Comment 24

[Luke Wagner [: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

Comment 25

[Branislav Rankov [:rankov]]

(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);

Comment 26

[Luke Wagner [:luke]]

(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"?

Comment 27

[Luke Wagner [:luke]]

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().

Comment 28

[Branislav Rankov [:rankov]]

(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.

Comment 29

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part1.patch

Rebased patch so that it can be applied.

Carry review from previous patch.

Comment 30

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part2.patch

Rebased the code after bug 860736 has landed. I think that this might need another review.

Comment 31

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part3.patch

This is a MIPS follow-up on changes done in bug 860736.

Comment 32

[Branislav Rankov [:rankov]]

Here is try link for these patches:
https://tbpl.mozilla.org/?tree=Try&rev=786ae4183028

Comment 33

[Luke Wagner [:luke]]

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?

Comment 34

[Luke Wagner [:luke]]

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!

Comment 35

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part2.patch

Added ShadowStackSpace to mentioned places.

Carry review from previous patch.

Comment 36

[Branislav Rankov [:rankov]]

Attachment: OdinMonkey-part3.patch

Fixed (callFromAsmJS) in MIPS and ARM.

Carry review from previous patch.

Comment 37

[Branislav Rankov [:rankov]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c69a670294fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7c59e556cc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b56930ff7e05

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c69a670294fa
https://hg.mozilla.org/mozilla-central/rev/f7c59e556cc6
https://hg.mozilla.org/mozilla-central/rev/b56930ff7e05