Last Comment Bug 617244 - nanojit: remove AvmCore
: nanojit: remove AvmCore
Status: RESOLVED FIXED
fixed-in-nanojit, fixed-in-tracemonkey
:
Product: Core Graveyard
Classification: Graveyard
Component: Nanojit (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks: 529430 559132 624604
  Show dependency treegraph
 
Reported: 2010-12-06 20:03 PST by Nicholas Nethercote [:njn]
Modified: 2014-03-17 08:00 PDT (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
NJ patch (against TM 58520:8921e3faccd2) (19.27 KB, patch)
2010-12-06 20:03 PST, Nicholas Nethercote [:njn]
edwsmith: review+
Details | Diff | Splinter Review
TM patch (against TM 58520:8921e3faccd2) (8.68 KB, patch)
2010-12-06 20:04 PST, Nicholas Nethercote [:njn]
gal: review+
Details | Diff | Splinter Review
TR patch (against TR 5632:00830e7731c4) (5.19 KB, patch)
2010-12-06 20:05 PST, Nicholas Nethercote [:njn]
edwsmith: review+
Details | Diff | Splinter Review
updated TR patch against 6063: f4943b15ab15 (2.79 KB, patch)
2011-03-10 18:40 PST, Rick Reitmaier
no flags Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2010-12-06 20:03:47 PST
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.
Comment 1 Nicholas Nethercote [:njn] 2010-12-06 20:04:34 PST
Created attachment 495752 [details] [diff] [review]
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 [:njn] 2010-12-06 20:05:24 PST
Created attachment 495753 [details] [diff] [review]
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 2010-12-06 21:31:03 PST
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 [:njn] 2010-12-06 21:47:39 PST
(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 2010-12-07 13:38:48 PST
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 7 Rick Reitmaier 2011-03-10 18:40:45 PST
Created attachment 518608 [details] [diff] [review]
updated TR patch against 6063: f4943b15ab15

rev TR patch
Comment 8 Tamarin Bot 2011-03-14 16:15:08 PDT
changeset: 6077:8e3c1b63f9fa
user:      Nicholas Nethercote <nnethercote@mozilla.com>
summary:   Bug 617244 - nanojit: remove AvmCore (NJ-specific part).  r=edwsmith.

http://hg.mozilla.org/tamarin-redux/rev/8e3c1b63f9fa
Comment 9 Tamarin Bot 2011-03-14 16:15:21 PDT
changeset: 6078:59c829fda638
user:      Nicholas Nethercote <nnethercote@mozilla.com>
summary:   Fix ARM breakage from bug 617244.  r=me.

http://hg.mozilla.org/tamarin-redux/rev/59c829fda638
Comment 10 Tamarin Bot 2011-03-14 16:16:34 PDT
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 [:njn] 2011-03-15 01:54:27 PDT
Does Tamarin Bot really need to duplicate earlier comments?  It's kind of annoying...

Note You need to log in before you can comment on or make changes to this bug.