574969 - add LIR_qasd / LIR_dasq

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Luke Wagner [:luke]]

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

[Luke Wagner [:luke]]

Oh, and the same issue comes up in reverse when boxing doubles, so a LIR_d2q would be appreciated as well.

Comment 2

[Nicholas Nethercote [inactive]]

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.

Comment 3

[Jeff Walden [:Waldo]]

pund2q and punq2d might also be plausible names.

Comment 4

[Nicholas Nethercote [inactive]]

(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

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch:

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

- 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 testlirc.sh 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
  machine.)

- 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".

Comment 6

[Nicholas Nethercote [inactive]]

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

[Luke Wagner [:luke]]

Awesome, thanks!

Comment 8

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/06774ab0e7e0

Comment 9

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/e3f0aafdd13b

Comment 10

[Edwin Smith]

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

Comment 11

[Edwin Smith]

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
 
 OP_UN(108)
 
 //---------------------------------------------------------------------------
 // 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

[Edwin Smith]

Attachment: asm differences (bottom-up listing)

Comment 13

[Edwin Smith]

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.

Comment 14

[Edwin Smith]

Discussion moved to bug 581702

Comment 15

[Edwin Smith]

TR: http://hg.mozilla.org/tamarin-redux/rev/d46ec128a3d8

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e3f0aafdd13b