557483 - nanojit: convert i386 codegen macros to functions

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

This is good in general -- better type-checking, no danger of forgetting to over-parenthesise arguments.  And it'll also help particularly with bug 506693, where having the extra type-checking on some of the arguments will avoid potential errors because the args are cast before use.

Comment 1

[Edwin Smith]

Comment on attachment 437271 [details] [diff] [review]
patch

Generally, great!

should these by typedefs?
#define R Register
#define I32 int32_t

IIRC, the inline keyword isn't required on method declarations, even if the definition is marked inline.  (but doesn't hurt, if you want to keep it for doc purposes)

I didn't review line by line for typos or cut+paste errors, counting on testing.

Comment 2

[Steven Johnson]

Painful as it may be, shouldn't we institute a REALLY_INLINE functionality as we've been using in TR? Otherwise we'll find that some compile config will helpfully ignore plain old "inline"...

Comment 3

[Edwin Smith]

maybe, but I haven't pushed on it, because:

* using out-of-line functions with the 'inline' keyword already avoids one of the problems, namely that the  use of 'inline' (or not) inside a class def wasn't treated uniformly.

* we haven't instituted a style where all methods are outside class defs.  (maybe we should, but imho thats a more important issue than REALLY_INLINE).

* to my knowlege, the performance of nanojit doesn't depend critically on any particular one of these functions being inlined.  if it did, i'd be for using REALLY_INLINE there.  but over-use of REALLY_INLINE is a recipe for bloat and eye strain.  I would want to see experimental data on code size and jit performance before committing to a policy in the code.

within TR, we probably err on the side of overuse of REALLY_INLINE in the -inlines.h files, but at least we're being consistent.

Comment 4

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/4062fae8baf2
http://hg.mozilla.org/projects/nanojit-central/rev/3607404c8ab9

http://hg.mozilla.org/tracemonkey/rev/4bf82590923b
http://hg.mozilla.org/tracemonkey/rev/2113638ab7cf

Comment 5

[Edwin Smith]

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

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/4bf82590923b