Closed
Bug 562744
Opened 14 years ago
Closed 6 years ago
Inline fastpath for atomToDouble
Categories
(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P3)
Tamarin Graveyard
Baseline JIT (CodegenLIR)
Tracking
(Not tracked)
RESOLVED
WONTFIX
Q2 12 - Cyril
People
(Reporter: wmaddox, Assigned: wmaddox)
References
Details
(Whiteboard: PACMAN, has-patch)
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
n.nethercote
:
review-
edwsmith
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
rreitmai
:
review+
edwsmith
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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.
Attachment #442498 -
Flags: review?(nnethercote)
Attachment #442498 -
Flags: feedback?(edwsmith)
Assignee | ||
Updated•14 years ago
|
Attachment #442497 -
Attachment is patch: true
Attachment #442497 -
Flags: review?(edwsmith)
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Updated•14 years ago
|
Attachment #442498 -
Attachment is patch: true
Assignee | ||
Updated•14 years ago
|
Whiteboard: PACMAN
Comment 2•14 years ago
|
||
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.
Assignee | ||
Comment 3•14 years ago
|
||
(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.
Assignee | ||
Comment 4•14 years ago
|
||
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.
Updated•14 years ago
|
Summary: Inline fastpath for conversion of atom to double → Inline fastpath for atomToDouble
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.2
Comment 5•14 years ago
|
||
Comment on attachment 442498 [details] [diff] [review]
Add q2d instruction to Nanojit (presently for x86_64 only)
Looks fine to me.
Attachment #442498 -
Flags: feedback?(edwsmith) → feedback+
Updated•14 years ago
|
Attachment #442497 -
Flags: review?(edwsmith)
Comment 6•14 years ago
|
||
Comment on attachment 442497 [details] [diff] [review]
Inline fastpath for conversion of atom to double
removing review pending resolution of bug 563944
Updated•14 years ago
|
Whiteboard: PACMAN → PACMAN, has-patch
Updated•14 years ago
|
Component: Virtual Machine → JIT Compiler (NanoJIT)
QA Contact: vm → nanojit
Comment 7•14 years ago
|
||
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.
Updated•14 years ago
|
Attachment #442498 -
Flags: review?(nnethercote) → review-
Assignee | ||
Comment 8•14 years ago
|
||
Rebased. Temporarily removed dependence on q2d instruction.
Attachment #442497 -
Attachment is obsolete: true
Assignee | ||
Comment 9•14 years ago
|
||
This doesn't require any NanoJIT extensions. Let's land it first.
Attachment #479498 -
Flags: superreview?(edwsmith)
Attachment #479498 -
Flags: review?(rreitmai)
Comment 10•14 years ago
|
||
Comment on attachment 479498 [details] [diff] [review]
Inline fastpath for conversion of atom to double (32-bit only)
r+ works for me
Attachment #479498 -
Flags: review?(rreitmai) → review+
Comment 11•14 years ago
|
||
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.
Attachment #479498 -
Flags: superreview?(edwsmith) → superreview+
Assignee | ||
Comment 12•14 years ago
|
||
Pushed to tamarin-redux:
http://hg.mozilla.org/tamarin-redux/rev/a394bcf0aad4
Updated•14 years ago
|
Flags: flashplayer-injection-
Assignee | ||
Comment 14•14 years ago
|
||
(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.
Status: NEW → ASSIGNED
Comment 15•13 years ago
|
||
Retargeting for Brannan.
Target Milestone: Q3 11 - Serrano → Q1 12 - Brannan
Comment 16•6 years ago
|
||
Tamarin is a dead project now. Mass WONTFIX.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Comment 17•6 years ago
|
||
Tamarin isn't maintained anymore. WONTFIX remaining bugs.
You need to log in
before you can comment on or make changes to this bug.
Description
•