Closed Bug 606066 Opened 12 years ago Closed 12 years ago

nanojit: some i386 backend clean-ups

Categories

(Core Graveyard :: Nanojit, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

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

Attachments

(1 file)

This patch cleans up the i386 back-end a bit more.

- MODRMsib() and MODRMs() do exactly the same thing, even though they look 
  different (except the argument order is different).  This patch removes
  MODRMs(), but changes MODRMsib to look more like MODRMs() did.

- This patch also changes code like this:

    _nIns -= 2;
    _nIns[0] = uint8_t(c>>8);
    _nIns[1] = uint8_t(c);

  to this:

    *(--_nIns) = uint8_t(c);
    *(--_nIns) = uint8_t(c>>8);

  which is the style used in most of the rest of the file.

- This patch renamed MUL as IMUL, which matches the i386 opcode naming.  ("mul"
  is quite different and much more limited than "imul".)
Attachment #484935 - Flags: review?(rreitmai)
(In reply to comment #0)
> - This patch also changes code like this:
> 
>     _nIns -= 2;
>     _nIns[0] = uint8_t(c>>8);
>     _nIns[1] = uint8_t(c);
> 
>   to this:
> 
>     *(--_nIns) = uint8_t(c);
>     *(--_nIns) = uint8_t(c>>8);

Agreed that consistency is good, but I suspect that the first style actually generates better / more compact code, at least on x86.
(In reply to comment #1)
> (In reply to comment #0)
> > - This patch also changes code like this:
> > 
> >     _nIns -= 2;
> >     _nIns[0] = uint8_t(c>>8);
> >     _nIns[1] = uint8_t(c);
> > 
> >   to this:
> > 
> >     *(--_nIns) = uint8_t(c);
> >     *(--_nIns) = uint8_t(c>>8);
> 
> Agreed that consistency is good, but I suspect that the first style actually
> generates better / more compact code, at least on x86.

I don't think native codegen is hot enough for it to matter.  Also, I plan to file follow-up bugs where most of these cases are replaced with calls to new functions like SIB(), MODRM(), OPCODE(), so there'll only be a few places where this is relevant.  We can argue about it again then :)
Comment on attachment 484935 [details] [diff] [review]
patch (against TM 55696:51ca3657de5b)

Agreed, performance of codegen rarely shows up on the radar and the value of consistency cannot be understated.
Attachment #484935 - Flags: review?(rreitmai) → review+
http://hg.mozilla.org/mozilla-central/rev/53ddd7c5944b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/tamarin-redux/rev/3bb939d37954
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.