Closed Bug 486535 Opened 12 years ago Closed 12 years ago

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


(Core :: JavaScript Engine, enhancement)

Not set





(Reporter: jbramley, Assigned: jbramley)


(Whiteboard: fixed-in-tracemonkey)


(1 file, 2 obsolete files)

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.
Attachment #370670 - Flags: review?(vladimir)
Fixed patch which actually deletes the relevant files.
Attachment #370670 - Attachment is obsolete: true
Attachment #370672 - Flags: review?(vladimir)
Attachment #370670 - Flags: review?(vladimir)
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.
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!

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 :-)

Attachment #370672 - Attachment is obsolete: true
Attachment #384069 - Flags: review?(gal)
Attachment #370672 - Flags: review?(vladimir)
Attachment #384069 - Flags: review?(gal) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: fixed-in-tracemonkey
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.