Status

Core Graveyard
Nanojit
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.
Attachment #495751 - Flags: review?(edwsmith)
(Assignee)

Comment 1

7 years ago
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.
Attachment #495752 - Flags: review?(gal)
(Assignee)

Comment 2

7 years ago
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.
Attachment #495753 - Flags: review?(edwsmith)
(Assignee)

Updated

7 years ago
Attachment #495752 - Attachment description: TMJ patch (against TM 58520:8921e3faccd2) → TM patch (against TM 58520:8921e3faccd2)

Comment 3

7 years ago
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.
Attachment #495752 - Flags: review?(gal) → review+
(Assignee)

Comment 4

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

Updated

7 years ago
Attachment #495751 - Flags: review?(edwsmith) → review+

Updated

7 years ago
Attachment #495753 - Flags: review?(edwsmith) → review+

Comment 5

7 years ago
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).
(Assignee)

Updated

7 years ago
Blocks: 624604
(Assignee)

Updated

7 years ago
Blocks: 559132
(Assignee)

Comment 6

7 years ago
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
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey

Comment 7

7 years ago
Created attachment 518608 [details] [diff] [review]
updated TR patch against 6063: f4943b15ab15

rev TR patch
Attachment #495753 - Attachment is obsolete: true

Comment 8

7 years ago
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

7 years ago
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

7 years ago
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
(Assignee)

Comment 11

7 years ago
Does Tamarin Bot really need to duplicate earlier comments?  It's kind of annoying...
(Assignee)

Comment 12

6 years ago
http://hg.mozilla.org/mozilla-central/rev/0c9a27c64492
http://hg.mozilla.org/mozilla-central/rev/d600268eeba6
http://hg.mozilla.org/mozilla-central/rev/40f500ecdfd0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.