Closed Bug 538060 Opened 10 years ago Closed 10 years ago

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

Categories

(Core Graveyard :: Nanojit, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
flash10.1

People

(Reporter: njn, Assigned: njn)

References

Details

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

Attachments

(2 files)

Attached patch NJ patchSplinter Review
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.
Attachment #420235 - Flags: review?(gal)
Attached patch TR patchSplinter Review
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.
Attachment #420236 - Flags: review?(rreitmai)
Attachment #420235 - Flags: review?(rreitmai)
Comment on attachment 420235 [details] [diff] [review]
NJ patch

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

???
Attachment #420235 - Flags: review?(gal) → review+
(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.
No, I don't mind it to stay in, just explain a bit better what it means.
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
Attachment #420236 - Flags: review?(rreitmai) → review+
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.
Attachment #420235 - Flags: review?(rreitmai)
Attachment #420235 - Flags: review?(edwsmith)
Attachment #420235 - Flags: review+
Attachment #420235 - Flags: review?(edwsmith) → review+
Duplicate of this bug: 536291
Duplicate of this bug: 520716
http://hg.mozilla.org/tracemonkey/rev/56cdca9fe3d8
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
http://hg.mozilla.org/tamarin-redux/rev/bacd29f05f1a
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Depends on: 539379
Target Milestone: --- → flash10.1
http://hg.mozilla.org/mozilla-central/rev/56cdca9fe3d8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.