Closed Bug 498193 Opened 13 years ago Closed 12 years ago

nanojit: inline all LirWriter functions

Categories

(Core Graveyard :: Nanojit, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gal, Assigned: n.nethercote)

Details

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

Attachments

(1 file, 1 obsolete file)

No description provided.
This is a series of patches rolled into one. I can split them out if that's preferred:

- Remove the highly specific int32 addition by doing 16-bit adds and shifts detection code we added back when. It doesn't seem to help perf any more.
- Remove dead CallInfo* functions parameter from lirbuf, filters, and a few other places.
- Add a flags field to LIRopcodes.tbl that indicates the class of the opcode (GUARD,BRANCH,LOAD,STORE,LIVE,RET etc) as well as whether the opcode can be CSE-ed or not. This is less brittle than the >= int <= uge comparison. It restores CSE for a bunch of opcodes that weren't covered before (i2f, u2f, addp).
- Make a bunch of short LIR functions inline and clean up naming a bit.
- Use opcode() instead of firstWord.code all over the place.
- Use new opcode class bitmask matching instead of long chains of comparisons.
- Use enum values instead of #defines for LIR_*p renamings (fixes a bug, one of the cases was out of date with the latest renamings of LIR_st/LIR_sti/LIR_stqi).
- Make manipulation functions for reservations methods of struct Reservation.
- Make convenience functions in LirWriter inline, most the time they just shuffle arguments around.
- Make CALL, CALLr and RET methods of Assembler instead of macros to avoid a name clash. We should probably do this for the rest of the macros as well. They are currently exposed into the nanojit namespace for no apparent reason.
Attached patch patch (obsolete) — Splinter Review
Assignee: general → gal
Attachment #383150 - Flags: review?(nnethercote)
Splitting the patches would make things easier, thanks.
(In reply to comment #1)
> This is a series of patches rolled into one. I can split them out if that's
> preferred:

Almost all of this stuff has been done elsewhere by now.

> - Remove the highly specific int32 addition by doing 16-bit adds and shifts
> detection code we added back when. It doesn't seem to help perf any more.

This is TM-specific, we could still do it.

> - Remove dead CallInfo* functions parameter from lirbuf, filters, and a few
> other places.

Done elsewhere, I think.

> - Add a flags field to LIRopcodes.tbl that indicates the class of the opcode
> (GUARD,BRANCH,LOAD,STORE,LIVE,RET etc) as well as whether the opcode can be
> CSE-ed or not. This is less brittle than the >= int <= uge comparison. It
> restores CSE for a bunch of opcodes that weren't covered before (i2f, u2f,
> addp).

Done elsewhere.

> - Make a bunch of short LIR functions inline and clean up naming a bit.

Probably done elsewhere.

> - Use opcode() instead of firstWord.code all over the place.

Done elsewhere.

> - Use new opcode class bitmask matching instead of long chains of comparisons.

More or less done elsewhere.

> - Use enum values instead of #defines for LIR_*p renamings (fixes a bug, one of
> the cases was out of date with the latest renamings of
> LIR_st/LIR_sti/LIR_stqi).

Done elsewhere.

> - Make manipulation functions for reservations methods of struct Reservation.

Moot, reservations are dead now.

> - Make convenience functions in LirWriter inline, most the time they just
> shuffle arguments around.

Not done.

> - Make CALL, CALLr and RET methods of Assembler instead of macros to avoid a
> name clash. We should probably do this for the rest of the macros as well. They
> are currently exposed into the nanojit namespace for no apparent reason.

Not done.

Andreas, you want to close this bug, or specify the things you'd still like?
I think the inlining might be worth a look still. Want to file a bug? CALL/CALLr/RET seems very low priority.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to comment #5)
> I think the inlining might be worth a look still. Want to file a bug?

I'll just refocus this one.

> CALL/CALLr/RET seems very low priority.

Agreed.
Assignee: gal → nnethercote
Status: RESOLVED → REOPENED
Component: JavaScript Engine → Nanojit
QA Contact: general → nanojit
Resolution: FIXED → ---
Summary: TM: [nanojit] another round of code cleanups → nanojit: inline all LirWriter functions
This inlines all of them except insStorei() and ins_choose(), which are big.  Also, ins_choose() had the capability of taking a non-condition as the first arg and turning it into a condition.  But neither TM nor TR ever used it, and it feels dodgy to me, so I removed it.

Net effect of the patch is slight reductions in code size and instructions executed.
Attachment #432500 - Flags: review?(gal)
Comment on attachment 432500 [details] [diff] [review]
LirWriter inlining patch

Cool. Thanks for looking into this.
Attachment #432500 - Flags: review?(gal) → review+
(In reply to comment #1)
> >
> > - Remove the highly specific int32 addition by doing 16-bit adds and shifts
> > detection code we added back when. It doesn't seem to help perf any more.
> 
> This is TM-specific, we could still do it.

See bug 552582.
(In reply to comment #7)

> Also, ins_choose() had the capability of taking a non-condition as the first
> arg and turning it into a condition.  But neither TM nor TR ever used it, and
> it feels dodgy to me, so I removed it.

I vaguely remember having to deal with non-condition "cond" parameters in the situation when a condition gets const folded to true or false.  If we don't see it in testing, fine, and if we run into that, we can explicitly handle constants (but not arbitrary expressions) or deal with it upstream.
(In reply to comment #11)
>
> I vaguely remember having to deal with non-condition "cond" parameters in the
> situation when a condition gets const folded to true or false.  If we don't see
> it in testing, fine, and if we run into that, we can explicitly handle
> constants (but not arbitrary expressions) or deal with it upstream.

ins_choose() should only be called at the very start of the pipeline, before any constant folding occurs, and it's immediately decomposed into simpler insXyz() calls.  So I think there won't be a problem.
(In reply to comment #12)
> > situation when a condition gets const folded to true or false.  If we don't see
> > it in testing, fine, and if we run into that, we can explicitly handle
> > constants (but not arbitrary expressions) or deal with it upstream.
> 
> ins_choose() should only be called at the very start of the pipeline, before
> any constant folding occurs, and it's immediately decomposed into simpler
> insXyz() calls.  So I think there won't be a problem.

I ran into the problem in TR, the condition passed into ins_choose was the output from ins2(...) that was constant folded.  In my case this was easy to work around by just optimizing away before calling ins_choose.

Note that ExprFilter::ins3() does the same optimization: it allows oprnd1, the condition, to be a constant.  this probably argues for ins_choose to also allow the condition to be constant, but I dont feel strongly about it.
Yeah, I was completely wrong about ins_choose().  Although it's right at the start of the pipeline, the condition will have gone all the way through the pipeline (and thus been optimized) before ins_choose() sees it.  I've reinstated the old code, but with an assertion that it's either a condition or a const.

NJ: http://hg.mozilla.org/projects/nanojit-central/rev/c99da0023362
TM: http://hg.mozilla.org/tracemonkey/rev/6eedb7f2c6d1
http://hg.mozilla.org/mozilla-central/rev/e65064a240b1
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Attachment #383150 - Attachment is obsolete: true
Attachment #383150 - Flags: review?(nnethercote)
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.