1024515 - IonMonkey MIPS: Test basic/testDivModWithIntMin.js fails on MIPS

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Branislav Rankov [:rankov]]

Test basic/testDivModWithIntMin.js fails on MIPS with the message:

/scratch/work/github-gecko/gecko-dev/js/src/jit-test/tests/basic/testDivModWithIntMin.js:25:4 Error: Assertion failed: got 0, expected -1

It fails with following switches:
--ion-eager --ion-parallel-compile=off 
--ion-eager --ion-parallel-compile=off --ion-check-range-analysis --no-sse3

Comment 1

[Branislav Rankov [:rankov]]

Attachment: mod-mask-i.patch

There was a bug in ma_mod_mask. When mask is larger than 16 bits, ScratchRegister is clobbered. I fixed this by using a new register instead of scratch.

Comment 2

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8448103 [details] [diff] [review]
mod-mask-i.patch

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

The code here looks fine.  Two very optional considerations:
1) It looks like the code that you generate now is identical to the old code, but never uses the ScratchRegister as a temp variable.  Theoretically, it is easier on the register allocator if you only take the extra scratch register when you know the mask will be over 1<<16. You can request the register allocator not give a register in that case, then at the top of ma_mod_mask, set remain = ScratchRegister
2) slightly different direction.  if mask > (1>>16), you take a scratch register, and repeatedly set it to mask.  You can instead assign it at the beginning of ma_mod_mask, then just use it without modifying it for the rest of the code generated for ma_mod_mask.

Comment 3

[Branislav Rankov [:rankov]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #2)
> Comment on attachment 8448103 [details] [diff] [review]
> mod-mask-i.patch
> 
> Review of attachment 8448103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code here looks fine.  Two very optional considerations:
> 1) It looks like the code that you generate now is identical to the old
> code, but never uses the ScratchRegister as a temp variable.  Theoretically,
> it is easier on the register allocator if you only take the extra scratch
> register when you know the mask will be over 1<<16. You can request the
> register allocator not give a register in that case, then at the top of
> ma_mod_mask, set remain = ScratchRegister
> 2) slightly different direction.  if mask > (1>>16), you take a scratch
> register, and repeatedly set it to mask.  You can instead assign it at the
> beginning of ma_mod_mask, then just use it without modifying it for the rest
> of the code generated for ma_mod_mask.

Thanks for the suggestions. I will leave it as it is for now. I will consider doing these improvements at a later time. I'm planing to reconsider if ma_mod_mask is worth using on MIPS since MIPS has div/mod instruction that might be faster than ma_mod_mask anyway.

Comment 4

[Branislav Rankov [:rankov]]

Here is the try link for the patch:
https://tbpl.mozilla.org/?tree=Try&rev=2767a212b2be

Comment 5

[Branislav Rankov [:rankov]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/401dd194ab8c

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/401dd194ab8c