TraceMonkey: ARM Thumb nanojit is not maintained and should be deleted.

RESOLVED FIXED

Status

()

--
enhancement
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: jbramley, Assigned: jbramley)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
This has been discussed (briefly) on #jsapi. The NativeThumb nanojit (for the ARM target) has not been maintained for some time and it doesn't seem to be possible to configure the build to include it. I have therefore made a tiny patch to remove it from Trace Monkey.

If Thumb(-2) support is required in the future, it would probably be better to port the existing NativeARM nanojit as it has optimizations and fixes which would probably be applicable to Thumb(-2) as well as ARM.
(Assignee)

Comment 1

10 years ago
Created attachment 370670 [details] [diff] [review]
Remove Thumb nanojit files and references.
Attachment #370670 - Flags: review?(vladimir)
(Assignee)

Comment 2

10 years ago
Created attachment 370672 [details] [diff] [review]
Remove Thumb nanojit files and references.

Fixed patch which actually deletes the relevant files.
Attachment #370670 - Attachment is obsolete: true
Attachment #370672 - Flags: review?(vladimir)
Attachment #370670 - Flags: review?(vladimir)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
I'd like to see this landed.  The patch is pretty simple and looks good to me, although I'm not an expert.
I'll reiterate that I'd like this to be landed -- when changing LIR and having to update the back-ends, it's annoying that the thumb back-end is there and must be ignored.

But I think there's more to be taken out.  If I grep for "thumb" I can see numerous things that should be removable in nanojit/avmplus.h, jstracer.cpp, and nanojit/NativeARM.cpp.
Summary: TraceMonkey: Thumb nanojit is not maintained and should be deleted. → TraceMonkey: ARM Thumb nanojit is not maintained and should be deleted.
(Assignee)

Comment 5

10 years ago
Nope, you can't remove them!

---

avmplus.h includes configuration flags (for AvmCore::config) which indicate whether or not Thumb and Thumb-2 are supported. Thumb support indicates that we need to generate interworking branches when calling native code* (as in bug 486639). Thumb-2 support is used to identify ARMv6T2 and above**, which support a few extra instructions that the JIT can use. Thus, even though we don't generate Thumb-2 code, there are still ARM instructions which were introduced with Thumb-2.

* This is interesting as we have since decided that we don't need to support ARMv4, and everything newer than that does support Thumb. Still, it's probably worth keeping the configuration flag as a sanity check.

** Actually the Linux detection code doesn't quite do this (due to auxv limitations) and it only detects Thumb-2 support for ARMv7 and above, but "if(thumb2)" will work with WinCE's platform detection code and it is more correct than "if(armv7)".

---

NativeARM.cpp contains an if(thumb2) to check whether or not it can use MOVW and MOVT. These were introduced with ARMv6T2 (as described above).

---

NativeARM.h contains two instances:
 * The first is a comment on the MOVZX8 macro, which is left over from the i386 JIT and does nothing on ARM. I removed this in bug 488845, so I wouldn't worry about it for now.
 * The second instance is a comment on the #endif at the end of the file, but this is already corrected by my patch for this bug.

---

Native.h contains two instances which are already removed by my patch.

---

Nativei386.h contains "Thumb" in a comment on line 452. I don't know x86 assembly at all, but I would guess that it shouldn't be there.

---

All other hits are in NativeThumb.cpp.

I think that explains all existing references to Thumb. Shout if something doesn't make sense!

Thanks,
Jacob
(Assignee)

Comment 6

10 years ago
Created attachment 384069 [details] [diff] [review]
Removes Thumb support. Re-synchronized with the tip.

Andreas, I've tagged you for review based on the e-mail discussion. Nicholas, if you want to steal the review, do so; it makes no difference to me :-)

Thanks,
Jacob
Attachment #370672 - Attachment is obsolete: true
Attachment #384069 - Flags: review?(gal)
Attachment #370672 - Flags: review?(vladimir)

Updated

10 years ago
Attachment #384069 - Flags: review?(gal) → review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed

Comment 7

10 years ago
http://hg.mozilla.org/tracemonkey/rev/1b5584a4eba4
Keywords: checkin-needed
Whiteboard: fixed-in-tracemonkey

Comment 9

10 years ago
http://hg.mozilla.org/mozilla-central/rev/7645eb3f210f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.