538060 - nanojit: improve 64-bit loads and stores in the X64 back-end

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: NJ patch

This patch:
- Takes advantage of the newly created ldq/stqi vs. ldf/stfi split to
  simplify and improve the X64 back-end.  In asm_load64() we previously
  always assumed a loaded 64-bit value was a float64, then did an XMM->GP
  move afterwards if necessary.  This improved the codegen for LIR_ldf, eg.
  this:

       $stack9 = ldf sp[-8]
         movq r12, -8(r13)
         movq xmm1, r12

  is now this:

       $stack9 = ldf sp[-8]
         movsd xmm1, -8(r13)

  As an example, for 3d-cube.js we execute 1% fewer native instructions
  because of this, although the difference was in the noise for SunSpider,
  probably because reg-to-reg moves are cheap.

  In asm_store64() the old code avoided this sub-optimality by considering
  the opcode and the type of the stored value, but it was really hacky.  The
  new code does it in a much cleaner way.  The codegen is unchanged except
  that the register allocation changes in a non-significant way, ie. some
  values end up in different registers, but there's no extra spilling or
  anything.

- Removes isFloat(), which was bogus -- it didn't include
  LIR_{fmod,float,ldf,ldfc,ld32f,ldc32f}.  Introduces isVoid(), isI32(),
  isI64(), isF64(), which are based on the retType[] and so will stay
  correct if new opcodes are added in the future.  Redefines isQuad() in
  terms of these, and moves them all together in LIR.h.  Only isF64() is
  currently used, but the others will be helpful in the near future.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: TR patch

This is pretty trivial.  It could be done better because isQuad() and isF64() overlap in meaning, but I did it as a purely mechanical change to keep it simple.

Comment 2

[Andreas Gal :gal]

Comment on attachment 420235 [details] [diff] [review]
NJ patch

>+        // XXX: i386 backend looks for value->isconst(), treats that specially
>+

???

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> (From update of attachment 420235 [details] [diff] [review])
> >+        // XXX: i386 backend looks for value->isconst(), treats that specially
> >+
> 
> ???

It's just a reminder for the future... there are some similar comments elsewhere in NJ, but I can take it out.

Comment 4

[Andreas Gal :gal]

No, I don't mind it to stay in, just explain a bit better what it means.

Comment 5

[Brendan Eich [:brendan]]

Protocol we use lately, I heard of it from Nat Friedman of Ximian (Novell now): use a FIXME: bug nnnnnn and file the bug now.

/be

Comment 6

[Rick Reitmaier]

Comment on attachment 420235 [details] [diff] [review]
NJ patch

Looks good to me but pulling Ed into the review loop since he's more familiar with the x64 back end.

Comment 7

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/1fd416d0954d

Comment 10

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/56cdca9fe3d8

Comment 11

[Steven Johnson]

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

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/56cdca9fe3d8