498193 - nanojit: inline all LirWriter functions

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Comment 1

[Andreas Gal :gal]

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.

Comment 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Nicholas Nethercote [inactive]]

Splitting the patches would make things easier, thanks.

Comment 4

[Nicholas Nethercote [inactive]]

(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?

Comment 5

[Andreas Gal :gal]

I think the inlining might be worth a look still. Want to file a bug? CALL/CALLr/RET seems very low priority.

Comment 6

[Nicholas Nethercote [inactive]]

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

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: LirWriter inlining patch

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.

Comment 8

[Andreas Gal :gal]

Comment on attachment 432500 [details] [diff] [review]
LirWriter inlining patch

Cool. Thanks for looking into this.

Comment 9

[Nicholas Nethercote [inactive]]

NJ: http://hg.mozilla.org/projects/nanojit-central/rev/63d6441f969c
TM: http://hg.mozilla.org/tracemonkey/rev/e65064a240b1

Comment 10

[Nicholas Nethercote [inactive]]

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

Comment 11

[Edwin Smith]

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

Comment 12

[Nicholas Nethercote [inactive]]

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

Comment 13

[Edwin Smith]

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

Comment 14

[Nicholas Nethercote [inactive]]

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

Comment 15

[Edwin Smith]

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

Comment 16

[Robert Sayre]

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