1434843 - [MIPS] Cleanup integer <-> floating point conversions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P5)

Description

[Dragan Mladjenovic]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36

Comment 1

[Dragan Mladjenovic]

Attachment: bug1434843_scratch.patch

Comment 2

[Dragan Mladjenovic]

Attachment: bug1434843_trunc.patch

Comment 3

[Dragan Mladjenovic]

Attachment: bug1434843_maybe.patch

Comment 4

[Dragan Mladjenovic]

Attachment: bug1434843_clamp.patch

Comment 5

[Dragan Mladjenovic]

Attachment: bug1434843_inexact.patch

Comment 6

[yuyin]

Comment on attachment 8947388 [details] [diff] [review]
bug1434843_maybe.patch

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

::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +741,5 @@
> +    moveFromDouble(ScratchDoubleReg, dest);
> +    ma_ext(ScratchRegister, ScratchRegister, Assembler::CauseV, 1);
> +    ma_b(ScratchRegister, Imm32(0), fail, Assembler::NotEqual);
> +
> +    as_sll(dest, dest, 0);

sign extend?

@@ +753,5 @@
> +    moveFromDouble(ScratchDoubleReg, dest);
> +    ma_ext(ScratchRegister, ScratchRegister, Assembler::CauseV, 1);
> +    ma_b(ScratchRegister, Imm32(0), fail, Assembler::NotEqual);
> +
> +    as_sll(dest, dest, 0);

sorry, I am not quite understand the code, but I just check arm64 & x64, they are zero extend?

Comment 7

[Dragan Mladjenovic]

(In reply to yuyin from comment #6)
> Comment on attachment 8947388 [details] [diff] [review]
> bug1434843_maybe.patch
> 
> Review of attachment 8947388 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
> @@ +741,5 @@
> > +    moveFromDouble(ScratchDoubleReg, dest);
> > +    ma_ext(ScratchRegister, ScratchRegister, Assembler::CauseV, 1);
> > +    ma_b(ScratchRegister, Imm32(0), fail, Assembler::NotEqual);
> > +
> > +    as_sll(dest, dest, 0);
> 
> sign extend?
> 
> @@ +753,5 @@
> > +    moveFromDouble(ScratchDoubleReg, dest);
> > +    ma_ext(ScratchRegister, ScratchRegister, Assembler::CauseV, 1);
> > +    ma_b(ScratchRegister, Imm32(0), fail, Assembler::NotEqual);
> > +
> > +    as_sll(dest, dest, 0);
> 
> sorry, I am not quite understand the code, but I just check arm64 & x64,
> they are zero extend?

Thank you for your review.

The comments state that this operation can be implemented as by doing 
conversion to int64 and then mod 2**32 which is effectively acts as zero extension,
but because int32 result has to be sign extended for mips64 we are doing the
sign extension here.

I'll put the comments for that.

Comment 8

[Dragan Mladjenovic]

Attachment: bug1434843_scratch2.patch

Comment 9

[Dragan Mladjenovic]

Attachment: bug1434843_trunc2.patch

Comment 10

[Dragan Mladjenovic]

Attachment: bug1434843_maybe2.patch

Comment 11

[Dragan Mladjenovic]

Attachment: bug1434843_clamp2.patch

Comment 12

[Dragan Mladjenovic]

Attachment: bug1434843_inexact2.patch

Comment 13

[Dragan Mladjenovic]

Attachment: bug1434843_scratch3.patch

Comment 14

[Dragan Mladjenovic]

Attachment: bug1434843_trunc3.patch

Comment 15

[Dragan Mladjenovic]

Attachment: bug1434843_maybe3.patch

Comment 16

[Dragan Mladjenovic]

Attachment: bug1434843_clamp3.patch

Comment 17

[Dragan Mladjenovic]

Attachment: bug1434843_inexact3.patch

Comment 18

[Dragan Mladjenovic]

Attachment: bug1434843_emu.patch

Comment 19

[Lars T Hansen [:lth]]

Comment on attachment 8953033 [details] [diff] [review]
bug1434843_emu.patch

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

If you say so :)  Looks plausible to me, though.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef2229b002f9
[MIPS] Remove SecondScratchDoubleReg; r=yuyin
https://hg.mozilla.org/integration/mozilla-inbound/rev/1214c83e6724
[MIPS] Cleanup wasm truncate ; r=yuyin
https://hg.mozilla.org/integration/mozilla-inbound/rev/57b5316a4c11
[MIPS] Cleanup MacroAssembler::branchTruncate*MaybeModUint32 ; r=yuyin
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9de0227411a
[MIPS] Make clampDoubleToUint8 less branchy; r=yuyin
https://hg.mozilla.org/integration/mozilla-inbound/rev/19eff992b4bb
[MIPS] Use FCSR to detect inexact conversion to integer; r=yuyin
https://hg.mozilla.org/integration/mozilla-inbound/rev/93b63582b679
[MIPS] Emulate FCRS Cause bits; r=

Comment 21

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/ef2229b002f9
https://hg.mozilla.org/mozilla-central/rev/1214c83e6724
https://hg.mozilla.org/mozilla-central/rev/57b5316a4c11
https://hg.mozilla.org/mozilla-central/rev/c9de0227411a
https://hg.mozilla.org/mozilla-central/rev/19eff992b4bb
https://hg.mozilla.org/mozilla-central/rev/93b63582b679