Closed Bug 1032786 Opened 7 years ago Closed 7 years ago

IonMonkey MIPS: Test asm.js/testFFI.js fails on MIPS

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: rankov, Assigned: rankov)

Details

Attachments

(2 files, 2 obsolete files)

Test asm.js/testFFI.js fails on MIPS with a segfault.

The failure manifested after the test was updated in bug 1024498
Assignee: nobody → branislav.rankov
Status: NEW → ASSIGNED
Part of the problem here is in AsmJS.cpp line: 
masm.convertValueToInt32(JSReturnOperand, ReturnFloatReg, ReturnReg, &oolConvert,
                                 /* -0 check */ false);

The problem is that JSReturnOperand is clobbered by this line because on MIPS ReturnReg is v0 and JSReturnOperand is (v0, v1). The oolConvert code expects JSReturnOperand to be intact.

I decided to change JSReturnOperand to (a2, a3) similar to what is done in ARM.

The other part of the problem is that arguments to CoerceInPlace_ToInt32 and CoerceInPlace_ToNumber are not properly aligned.
Attachment #8449496 - Flags: review?(luke)
Attachment #8449507 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8449496 [details] [diff] [review]
GenerateOOLConvert-alignment.patch

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

d'oh!
Attachment #8449496 - Flags: review?(luke) → review+
Comment on attachment 8449507 [details] [diff] [review]
Change-JSReturnOperand-regs.patch

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

::: js/src/jit/mips/Assembler-mips.h
@@ +103,5 @@
>  static MOZ_CONSTEXPR_VAR Register InvalidReg = { Registers::invalid_reg };
>  static MOZ_CONSTEXPR_VAR FloatRegister InvalidFloatReg = { FloatRegisters::invalid_freg };
>  
> +static MOZ_CONSTEXPR_VAR Register JSReturnReg_Type = a3;
> +static MOZ_CONSTEXPR_VAR Register JSReturnReg_Data = a2;

The way we write down Value in memory, we store the type first, then the payload, wouldn't it be faster to order the registers the same way, by using a2 for the type and a3 for the data?

::: js/src/jit/mips/Trampoline-mips.cpp
@@ +194,5 @@
>      if (type == EnterJitBaseline) {
>          // Handle OSR.
>          GeneralRegisterSet regs(GeneralRegisterSet::All());
>          regs.take(JSReturnOperand);
> +        regs.takeUnchecked(OsrFrameReg);

No, either we do things properly or we don't.  Do

  regs.add(OsrFrameReg);

  masm.call…

  regs.take(JSReturnOperand);

But takeUnchecked should not be used here, as this is basically saying don't bother I know what mess I am doing.
Attachment #8449507 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> The way we write down Value in memory, we store the type first, then the
> payload, wouldn't it be faster to order the registers the same way, by using
> a2 for the type and a3 for the data?

I don't think this is true for MIPS.
Attachment #8449507 - Attachment is obsolete: true
Attachment #8450966 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8450966 [details] [diff] [review]
Change-JSReturnOperand-regs.patch

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

Thanks :)
Attachment #8450966 - Flags: review?(nicolas.b.pierron) → review+
Nicolas, I have found a bug in previous patch and fixed it. Can you do a quick review?
Attachment #8450966 - Attachment is obsolete: true
Attachment #8451592 - Flags: review?(nicolas.b.pierron)
Attachment #8451592 - Flags: review?(nicolas.b.pierron) → review+
patch GenerateOOLConvert-alignment was made obsolete by patch simplify-ion-exit from bug 1034330
https://hg.mozilla.org/mozilla-central/rev/ed8b3cc1123b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.