562744 - Inline fastpath for atomToDouble

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P3)

Description

[William Maddox]

Attachment: Inline fastpath for conversion of atom to double

In CodegenLIR::coerceToNumber(), generate inline code to convert kIntptrType and kDoubleType atoms to a double.

This is the fourth in a series of patches based on the investigations reported in Bug 552542.

Comment 1

[William Maddox]

Attachment: Add q2d instruction to Nanojit (presently for x86_64 only)

Add q2d instruction, analogous to i2d on 64-bit platforms.

The result is undefined if the value of the quad integer exceeds the integer range of a double, i.e., cannot be represented exactly.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 442498 [details] [diff] [review]
Add q2d instruction to Nanojit (presently for x86_64 only)

>+
>+                case LIR_q2d:
>+                    countlir_fpu();
>+                    ins->oprnd1()->setResultLive();
>+                    if (ins->isExtant()) {
>+                        asm_q2f(ins);
>+                    }
>+                    break;

Should be 'asm_q2d', not 'asm_q2f'.


>+OP_64(q2d,     111, Op1,  D,    1)  // convert quad to double

The comment should have more detail about the exact semantics (eg. see d2i
for a comparison).

As for the semantics themselves -- if the int is not representable exactly
the result is undefined?  Undefined kinda sucks.  I looked at the intel
manual for the CVTSI2SD instruction (the 64-bit version), it was unclear
what it did in this situation.  I would guess that the result would be the
FP value closest to the integer.  In which case it would make sense for q2d
to have the same semantics.  I'm interested to know how this will be
implemented on PPC64.

A lazier option (mirroring d2i) would be to say "platform rounding rules
when exact conversions aren't possible", or similar, but we should be more
exact if we can.

Comment 3

[William Maddox]

(In reply to comment #2)
> (From update of attachment 442498 [details] [diff] [review])
> >+
> >+                case LIR_q2d:
> >+                    countlir_fpu();
> >+                    ins->oprnd1()->setResultLive();
> >+                    if (ins->isExtant()) {
> >+                        asm_q2f(ins);
> >+                    }
> >+                    break;
> 
> Should be 'asm_q2d', not 'asm_q2f'.

Agreed.  I've been trying not to get ahead of the overall renaming
efforts, however, and 'asm_i2f' is still hanging around.  I imagine
it will be gone by the time this patch lands, or perhaps I'll hasten
the process. ;)

> >+OP_64(q2d,     111, Op1,  D,    1)  // convert quad to double
> 
> The comment should have more detail about the exact semantics (eg. see d2i
> for a comparison).
> 
> As for the semantics themselves -- if the int is not representable exactly
> the result is undefined?  Undefined kinda sucks.  I looked at the intel
> manual for the CVTSI2SD instruction (the 64-bit version), it was unclear
> what it did in this situation.  I would guess that the result would be the
> FP value closest to the integer.  In which case it would make sense for q2d
> to have the same semantics.  I'm interested to know how this will be
> implemented on PPC64.

I wanted to preserve the possibility of a "bit-twiddling" implementation that
simply put an FP wrapper around the value without doing anything seriously mathematical.  I'll take a look at the other 64-bit architectures and see if this will be necessary for any of them.  In the Javascript/Actionscript context, the plausible use cases for integer->double conversions require an exact integer result, so my knee-jerk inclination was to specify no more than is needed.

Comment 4

[William Maddox]

PPC64 provides the 'fcfid' instruction, which converts a 64-bit signed integer to a double, rounding according to the rounding mode set in the FPSCR.  We currently use 'fcfid' following sign extension to handle the int32->double conversion.  On 32-bit PPC, bit-fiddling is required for the int32->double conversion.

SPARC V9 provides the similar 'fxtod' instruction, with rounding controlled by the FSR.

I think it is safe to specify that platform rounding rules apply.  A stronger specification would require that nanojit have control over the global rounding mode, which would imply costly mode switching or embedability issues.

Comment 5

[Edwin Smith]

Comment on attachment 442498 [details] [diff] [review]
Add q2d instruction to Nanojit (presently for x86_64 only)

Looks fine to me.

Comment 6

[Edwin Smith]

Comment on attachment 442497 [details] [diff] [review]
Inline fastpath for conversion of atom to double

removing review pending resolution of bug 563944

Comment 7

[Nicholas Nethercote [inactive]]

What's the status of this bug?  I'm marking my review as r- because I wasn't quite happy with the original patch and I'm cleaning my review queue.  I'm happy to review an updated patch if there is one.

Comment 8

[William Maddox]

Attachment: Inline fastpath for conversion of atom to double

Rebased.  Temporarily removed dependence on q2d instruction.

Comment 9

[William Maddox]

Attachment: Inline fastpath for conversion of atom to double (32-bit only)

This doesn't require any NanoJIT extensions.  Let's land it first.

Comment 10

[Rick Reitmaier]

Comment on attachment 479498 [details] [diff] [review]
Inline fastpath for conversion of atom to double (32-bit only)

r+ works for me

Comment 11

[Edwin Smith]

Comment on attachment 479498 [details] [diff] [review]
Inline fastpath for conversion of atom to double (32-bit only)

Overall: cool.

+#ifdef VMCFG_FASTPATH_FROMATOM
+            CodegenLabel not_intptr;
+            CodegenLabel not_double;
+            CodegenLabel done;
+            ...

It would really help to have a block comment here with lirasm pseudocode showing what is being inlined (or C++ pseudocode, if it tells the story better).  We're going to have a lot of these blocks, so +1 if you factor it into a function too.

Comment 12

[William Maddox]

Pushed to tamarin-redux:

http://hg.mozilla.org/tamarin-redux/rev/a394bcf0aad4

Comment 13

[Trevor Baker]

Can this be closed?

Comment 14

[William Maddox]

(In reply to comment #13)
> Can this be closed?

The bug remains open as the required support for 64-bit platforms has not landed in Nanojit, so the optimization is implemented only on 32-bit platforms.

Comment 15

[Dan Smith]

Retargeting for Brannan.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.