617244 - nanojit: remove AvmCore

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: NJ patch (against TM 58520:8921e3faccd2)

Something I came across while working on bug 529430:  there's this AvmCore
class that is used throughout Nanojit.  It's a Tamarin-ism, AFAICT, and
isn't needed, and it's quite confusing.  All that *is* needed is the Config
struct that AvmCore (statically) contains.  So I decided to clean this up.

The NJ patch:

- Removes AvmCore from the nanojit/avmplus.h file that TM and lirasm use
  (but TR doesn't).  Removes all traces of AvmCore elsewhere.

- Removes some other dead stuff from avmplus.h.

- nInit() is now passed no arguments;  none of the back-ends need an argument.

- Removes 'has_cmov' from the Sparc back-end;  it is dead.

- Moves use_cmov() from AvmCore to Config.

- Updates lirasm for all the above changes, by adding a nanojit::Config to
  class Lirasm.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: TM patch (against TM 58520:8921e3faccd2)

The TM patch:

- Adds a nanojit::Config variable to jstracer.cpp, which gets passed to
  Writer, just like LogController.

- Updates things for the NJ changes.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: TR patch (against TR 5632:00830e7731c4)

The TR patch:

- Updates for the NJ changes.

- Fixes a signed/unsigned comparison error that was preventing it from building.

Comment 3

[Andreas Gal :gal]

Comment on attachment 495752 [details] [diff] [review]
TM patch (against TM 58520:8921e3faccd2)

myConfig is a terrible name. How about just config? And instead of njConfig just config as well? Its obvious that its the nanojit config and its a member name. We can as well call it config.

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> Comment on attachment 495752 [details] [diff] [review]
> TM patch (against TM 58520:8921e3faccd2)
> 
> myConfig is a terrible name.

Yes it is!  Well spotted.  That was a temporary name that I later changed but looks like I didn't change it in the ARM-only code.  It's NJConfig in the rest of jstracer.cpp.

I like the NJ/nj prefix because it indicates that it's a configuration relating to Nanojit, not the rest of TM.

Comment 5

[Edwin Smith]

Nice change, I'd been wanting to do this for a while and kept getting distracted.  Reviewing the code rekindled my desire to eliminate the use_cmov configuration flag as well (bug 586472).

Comment 6

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/6a1b21753af6
http://hg.mozilla.org/projects/nanojit-central/rev/90782f292765

http://hg.mozilla.org/tracemonkey/rev/d600268eeba6
http://hg.mozilla.org/tracemonkey/rev/0c9a27c64492
http://hg.mozilla.org/tracemonkey/rev/2306f5bf50a2
http://hg.mozilla.org/tracemonkey/rev/40f500ecdfd0

Comment 7

[Rick Reitmaier]

Attachment: updated TR patch against 6063: f4943b15ab15

rev TR patch

Comment 8

[Tamarin Bot]

changeset: 6077:8e3c1b63f9fa
user:      Nicholas Nethercote <[e-mail]>
summary:   Bug 617244 - nanojit: remove AvmCore (NJ-specific part).  r=edwsmith.

http://hg.mozilla.org/tamarin-redux/rev/8e3c1b63f9fa

Comment 9

[Tamarin Bot]

changeset: 6078:59c829fda638
user:      Nicholas Nethercote <[e-mail]>
summary:   Fix ARM breakage from bug 617244.  r=me.

http://hg.mozilla.org/tamarin-redux/rev/59c829fda638

Comment 10

[Tamarin Bot]

changeset: 6086:fc8730a8d886
user:      Nicholas Nethercote <nnethercote>
summary:   Bug 617244 - nanojit: remove AvmCore (r=edwsmith)
.
. attachment 495753 [details] [diff] [review] - TR patch (against TR 5632:00830e7731c4)
.
Created attachment 495751 [details] [diff] [review]
NJ patch (against TM 58520:8921e3faccd2)

Something I came across while working on bug 529430:  there's this AvmCore
class that is used throughout Nanojit.  It's a Tamarin-ism, AFAICT, and
isn't needed, and it's quite confusing.  All that *is* needed is the Config
struct that AvmCore (statically) contains.  So I decided to clean this up.

The NJ patch:

- Removes AvmCore from the nanojit/avmplus.h file that TM and lirasm use
  (but TR doesn't).  Removes all traces of AvmCore elsewhere.

- Removes some other dead stuff from avmplus.h.

- nInit() is now passed no arguments;  none of the back-ends need an argument.

- Removes 'has_cmov' from the Sparc back-end;  it is dead.

- Moves use_cmov() from AvmCore to Config.

- Updates lirasm for all the above changes, by adding a nanojit::Config to
  class Lirasm.

http://hg.mozilla.org/tamarin-redux/rev/fc8730a8d886

Comment 11

[Nicholas Nethercote [inactive]]

Does Tamarin Bot really need to duplicate earlier comments?  It's kind of annoying...

Comment 12

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/mozilla-central/rev/0c9a27c64492
http://hg.mozilla.org/mozilla-central/rev/d600268eeba6
http://hg.mozilla.org/mozilla-central/rev/40f500ecdfd0