Core Graveyard
8 years ago
4 years ago


(Reporter: luke, Assigned: njn)


Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)


(2 attachments)



8 years ago
A LIR opcode that reinterprets the bits of a uint64 as a double would be useful with newjsvals when unboxing a double.  Without this op, the alternative is to perform two loads (first a LIR_ldq to do the guard and then a LIR_ldd to get the value), which isn't absolutely awful, but could be improved by the addition of this opcode.

Nick has graciously volunteered.

Comment 1

8 years ago
Oh, and the same issue comes up in reverse when boxing doubles, so a LIR_d2q would be appreciated as well.
Summary: add LIR_q2d → add LIR_q2d / LIR_d2q

Comment 2

8 years ago
Nb: the bug title originally suggested d2q/q2d for the opcode names.  But these would be a pure reinterpretation of the bits, rather than a conversion like d2i/i2d.  So dasq/qasd (ie. "D as Q", "Q as D") would be better names.
Summary: add LIR_q2d / LIR_d2q → add LIR_qasd / LIR_dasq

Comment 3

8 years ago
pund2q and punq2d might also be plausible names.

Comment 4

8 years ago
(In reply to comment #3)
> pund2q and punq2d might also be plausible names.

True, and that fits the pattern of the load-and-convert ones, eg. ldc2i.

Comment 5

8 years ago
Created attachment 455420 [details] [diff] [review]

This patch:

- Adds dasq/qasd.  They were pretty easy because asm_nongp_copy() already 

- Renames asm_promote() as asm_ui2uq(), for consistency with similar cases.

- Adds to ExprFilter::ins1() this optimisation: d2i(i2d(x)) == x.

- I didn't bother optimizing the dasq(immd) and qasd(immq) cases, partly
  because I was lazy, and partly because I doubt they'll come up in
  practice.  They can be added later if necessary.

- Adds infrastructure for 64-bit only lirasm tests.  They live in 64-bit/.  I
  added a --word-size option to lirasm which allows to detect
  whether it should run the 64-bit tests or not.  (Unlike the ARM-specific
  tests, 'uname' was no good because you can have 32-bit builds on a 64-bit

- Adds two tests for the new opcodes.

- Adds LIR_subq support to lirasm.

Nb: I decided against the punx2y names.  In the ldx2y case, you can read it as "load x and convert to y", but you can't do that with punx2y.  Whereas you can read xasy as "interpret x as y".
Attachment #455420 - Flags: review?(edwsmith)

Comment 6

8 years ago
Luke, it will make sense to not use these opcodes until fatvals has been merged to TM.  That will make things easier with the whole NJ-to-TM situation.

Comment 7

8 years ago
Awesome, thanks!


8 years ago
Blocks: 578917


8 years ago
Blocks: 578922


8 years ago
Attachment #455420 - Flags: review?(edwsmith) → review+

Comment 9

8 years ago
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey

Comment 10

8 years ago
Something in this NJ patch is breaking release builds of TR on x64, but not debug, or debug-debugger.  Still investigating.


8 years ago
Component: JavaScript Engine → Nanojit

Comment 11

8 years ago
It's due to the shuffling of LIR opcodes, but I still haven't found the smoking gun.  Here is a minimal shuffle that triggers segfault.  Note - bug is release-only; not due to optimization level, and goes away if I enable NJ_VERBOSE (but keep all other ifdefs at release settings).

Something related to ui2uq/q2i/i2q is going haywire.

diff -r a69e106004bc nanojit/LIRopcode.tbl
--- a/nanojit/LIRopcode.tbl	Sat Jul 24 10:01:52 2010 -0400
+++ b/nanojit/LIRopcode.tbl	Sat Jul 24 13:07:38 2010 -0400
@@ -295,18 +295,18 @@ OP___(cmovi,   106, Op3,  I,    1)  // c
 OP_64(cmovq,   107, Op3,  Q,    1)  // conditional move quad
 // Conversions
 OP_64(i2q,     109, Op1,  Q,    1)  // sign-extend int to quad
-OP_64(ui2uq,   110, Op1,  Q,    1)  // zero-extend unsigned int to unsigned quad
-OP_64(q2i,     111, Op1,  I,    1)  // truncate quad to int (removes the high 32 bits)
+OP_64(q2i,     110, Op1,  I,    1)  // truncate quad to int (removes the high 32 bits)
+OP_64(ui2uq,   111, Op1,  Q,    1)  // zero-extend unsigned int to unsigned quad
 OP___(i2d,     112, Op1,  D,    1)  // convert int to double
 OP___(ui2d,    113, Op1,  D,    1)  // convert unsigned int to double
 OP___(d2i,     114, Op1,  I,    1)  // convert double to int (no exceptions raised, platform rounding rules)

Comment 12

8 years ago
Created attachment 460052 [details] [diff] [review]
asm differences (bottom-up listing)

Comment 13

8 years ago
Looks like Assembler::nHints is uninitialized (from bug 513514), and this triggers subtle changes in register allocation which cause the crash.  

If I understand the hinting code correctly, bogus hints should not cause failures; so there could be a legitimate bug here.  Initializing nHints to 0 makes the crash go away.


8 years ago
Blocks: 581702

Comment 14

8 years ago
Discussion moved to bug 581702

Comment 15

8 years ago
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin

Comment 16

8 years ago
Last Resolved: 8 years ago
Resolution: --- → FIXED
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.