Closed Bug 1434843 Opened 2 years ago Closed 2 years ago

[MIPS] Cleanup integer <-> floating point conversions

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: dragan.mladjenovic, Assigned: dragan.mladjenovic)

References

Details

Attachments

(6 files, 10 obsolete files)

11.17 KB, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
43.37 KB, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
5.21 KB, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
5.07 KB, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
6.24 KB, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
5.71 KB, patch
lth
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36
Attached patch bug1434843_scratch.patch (obsolete) — Splinter Review
Attachment #8947386 - Flags: review?(yuyin-hf)
Attached patch bug1434843_trunc.patch (obsolete) — Splinter Review
Attachment #8947387 - Flags: review?(yuyin-hf)
Attached patch bug1434843_maybe.patch (obsolete) — Splinter Review
Attachment #8947388 - Flags: review?(yuyin-hf)
Attached patch bug1434843_clamp.patch (obsolete) — Splinter Review
Attachment #8947389 - Flags: review?(yuyin-hf)
Attached patch bug1434843_inexact.patch (obsolete) — Splinter Review
Attachment #8947390 - Flags: review?(yuyin-hf)
Depends on: wasm-mips
Attachment #8947386 - Flags: review?(yuyin-hf) → review+
Attachment #8947387 - Flags: review?(yuyin-hf) → review+
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?
Attachment #8947389 - Flags: review?(yuyin-hf) → review+
Attachment #8947390 - Flags: review?(yuyin-hf) → review+
(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.
Attachment #8947388 - Flags: review?(yuyin-hf) → review+
Blocks: 1435220
No longer blocks: 1435220
Depends on: 1435220
No longer depends on: 1435220
Depends on: 1435220
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P5
Attached patch bug1434843_scratch2.patch (obsolete) — Splinter Review
Attachment #8947386 - Attachment is obsolete: true
Attachment #8949435 - Flags: review+
Attached patch bug1434843_trunc2.patch (obsolete) — Splinter Review
Attachment #8947387 - Attachment is obsolete: true
Attachment #8949436 - Flags: review+
Attached patch bug1434843_maybe2.patch (obsolete) — Splinter Review
Attachment #8947388 - Attachment is obsolete: true
Attachment #8949438 - Flags: review+
Attached patch bug1434843_clamp2.patch (obsolete) — Splinter Review
Attachment #8947389 - Attachment is obsolete: true
Attachment #8949439 - Flags: review+
Attached patch bug1434843_inexact2.patch (obsolete) — Splinter Review
Attachment #8947390 - Attachment is obsolete: true
Attachment #8949440 - Flags: review+
No longer depends on: 1435220
Attachment #8949435 - Attachment is obsolete: true
Attachment #8953028 - Flags: review+
Attachment #8949436 - Attachment is obsolete: true
Attachment #8953029 - Flags: review+
Attachment #8949438 - Attachment is obsolete: true
Attachment #8953030 - Flags: review+
Attachment #8949439 - Attachment is obsolete: true
Attachment #8953031 - Flags: review+
Attachment #8949440 - Attachment is obsolete: true
Attachment #8953032 - Flags: review+
Attachment #8953033 - Flags: review?(lhansen)
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.
Attachment #8953033 - Flags: review?(lhansen) → review+
Keywords: leave-open
Pushed by dluca@mozilla.com:
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=
Keywords: checkin-needed
Assignee: nobody → dragan.mladjenovic
Depends on: 1465770
You need to log in before you can comment on or make changes to this bug.