Here is what can be read in Assembler::CountLeadingZeroes on 1.9.2 and trunk: // We can't do CLZ on anything earlier than ARMv5. Architectures as early // as that aren't supported, but assert that we aren't running on one // anyway. // If ARMv4 support is required in the future for some reason, we can do a // run-time check on config.arch and fall back to the C routine, but for // now we can avoid the cost of the check as we don't intend to support // ARMv4 anyway. It appears that while you may not intend to support ARMv4, Debian does, for openmoko support. I don't have a patch yet, but any input as to how you think this should be done is welcome.

There are almost certainly other areas of the ARM backend that will need attention here as well, it has assumed ARMv5 as the minimum for quite a while.

Yes there are. Albin Tonnerre wrote a patch addressing most of these areas that I need to adapt and test.

This patch is derived from the one I received from Albin Tonnerre, and is against 1.9.2. I unfortunately don't have access to an armv4t machine, but the toolchain on the armv5t i have access to targets armv4t and it works there. I also verified that it works properly (and correctly doesn't include the runtime checks, thanks to the ARM_ARCH_AT_LEAST macro) with -march=armv5t. With the patch applied, the js testsuite in 1.9.2 passes. (make check in js/src)

note ARM_ARCH_AT_LEAST could also be written #define ARM_ARCH_AT_LEAST(wanted) ((wanted <= __ARM_ARCH__) || (ARM_ARCH >= wanted)) which would make it better to use on trunk, where there are tests for _config.arm_arch >= 7. I won't have time to test on trunk for some time, though.

New version with a nicer ARM_ARCH_AT_LEAST macro.

Comment on attachment 442669 [details] [diff] [review] Nanojit support for armv4t on 1.9.2 >+ // Targetting cortex-r4 allows a toolchain with armv4t default target to >+ // still build with clz, and also allows Android gcc compiler to use it >+ // while it doesn't want with its default target. >+ if (ARM_ARCH_AT_LEAST(5)) >+ __asm (".cpu cortex-r4\n" This is the wrong way to go about it. A toolchain with ARMv4T default target should be given an appropriate CFLAGS so that it can emit ARMv5 code (or whatever you fancy). Inserting assembly directives like this into inline assembly is likely to cause headaches. (Also, why Cortex-R4? That's ARMv7-R, and will never really be a target for NanoJIT as it doesn't have an MMU.) I make ARMv7 (Cortex-A8) builds like this: export FLAGS_A8="-mcpu=cortex-a8 -mfpu=vfp -mfloat-abi=softfp" CFLAGS=$FLAGS_A8 CPPFLAGS=$FLAGS_A8 CXXFLAGS=$FLAGS_A8 ../configure [...] ---- I explicitly removed ARMv4T (and ARMv4) support from NanoJIT a while back for the following reasons: * I couldn't test it, and no-one else was doing so either. * The engine builds without the JIT for ARMv4T, so it doesn't break debian build systems. Those crazy enough to try to run Firefox on ARMv4T devices will probably tolerate the lack of a JIT anyway. * Maintaining the additional code paths was becoming a nuisance. If you want to add it back in, I think this should be discussed with people who have more clout than I do. It's certainly not something that we want to concentrate on. Having said that, it's not my decision, and for that reason I will defer the review to someone else (though my comment about Cortex-R4 still stands). I'm not saying that I don't value your efforts, and it is always nice to see someone working on the ARM back-end, but we agreed a while ago not to support ARMv4T and I don't think I can go back on the policy without confirmation from someone who knows! Thanks, Jacob

(In reply to comment #6) > (From update of attachment 442669 [details] [diff] [review]) > >+ // Targetting cortex-r4 allows a toolchain with armv4t default target to > >+ // still build with clz, and also allows Android gcc compiler to use it > >+ // while it doesn't want with its default target. > >+ if (ARM_ARCH_AT_LEAST(5)) > >+ __asm (".cpu cortex-r4\n" > > This is the wrong way to go about it. A toolchain with ARMv4T default target > should be given an appropriate CFLAGS so that it can emit ARMv5 code (or > whatever you fancy). Inserting assembly directives like this into inline > assembly is likely to cause headaches. I went this way because it has the double advantage of not requiring configure changes, which would be needed both on tracemonkey and nanojit, and working around the problem with the android ndk. > (Also, why Cortex-R4? That's ARMv7-R, and will never really be a target for > NanoJIT as it doesn't have an MMU.) (I took cortex-r4 because it was the first one in the -mcpu list that supported clz. i could have used, e.g. xscale, but then the android toolchain doesn't like it) > I make ARMv7 (Cortex-A8) builds like this: > > export FLAGS_A8="-mcpu=cortex-a8 -mfpu=vfp -mfloat-abi=softfp" > CFLAGS=$FLAGS_A8 CPPFLAGS=$FLAGS_A8 CXXFLAGS=$FLAGS_A8 ../configure [...] > > ---- > > I explicitly removed ARMv4T (and ARMv4) support from NanoJIT a while back for > the following reasons: > * I couldn't test it, and no-one else was doing so either. > * The engine builds without the JIT for ARMv4T, so it doesn't break debian > build systems. Those crazy enough to try to run Firefox on ARMv4T devices will > probably tolerate the lack of a JIT anyway. > * Maintaining the additional code paths was becoming a nuisance. (...) Debian builds only one set of packages for all arm cpus, and the current target is armv4t. Which means that building without JIT is not nice for those using more recent arm machines. (Also note that the engine *does* build with JIT when building with an armv4t target, but fails to build because of the clz opcode)

(In reply to comment #4) > #define ARM_ARCH_AT_LEAST(wanted) ((wanted <= __ARM_ARCH__) || (ARM_ARCH >= > wanted)) Sadly, I'm don't think all ARM compilers reliably define these symbols... see njcpudetect.h for the gyrations we go thru. (Debian GCC may always do so, but if we are going to add ARMv4 support back, in let's do it robustly.) (In reply to comment #7) > (Also note that the engine *does* build with JIT when > building with an armv4t target, but fails to build because of the clz opcode) The issue isn't whether the JIT will build properly (that's easily fixable, as you found); it's whether the JIT will emit code that is ARMv4 compliant -- if not, why bother?

(In reply to comment #7) > I went this way because it has the double advantage of not requiring configure > changes, which would be needed both on tracemonkey and nanojit, and working > around the problem with the android ndk. Ah, that makes sense to me now, though I'd go with something like ".arch armv5t" as it's semantically more sensible. It feels a bit wrong to put target-specifying directives in inline assembly; I don't know how the assembler handles .cpu and .arch directives in the middle of assembly input. However, I can't think of a better way to achieve the effect you're after, so your solution appears best. > Debian builds only one set of packages for all arm cpus, and the current target > is armv4t. Which means that building without JIT is not nice for those using > more recent arm machines. (Also note that the engine *does* build with JIT when > building with an armv4t target, but fails to build because of the clz opcode) I understand your pain :-) If ARMv4 is a compatibility & practicality target rather than a real target architecture, it might be worth trying to find a solution to this that allows us to target ARMv4T in the build but enable the JIT at run-time for ARMv5+. Modifying build-systems and infrastructure code is fiddly, but if it's the right thing to do then we should do it. If you want the JIT for ARMv4T too, there's no alternative but to put it in the back-end, of course! Hmm. Anyway, I'll let Mozilla comment on this. (The patch is good by the way, from a technical standpoint at least. With the Cortex-R4 thing swapped with "armv5t", it has an effective r+ from me.) (In reply to comment #8) > The issue isn't whether the JIT will build properly (that's easily fixable, as > you found); it's whether the JIT will emit code that is ARMv4 compliant -- if > not, why bother? It's perfectly valid to statically emit ARMv5 code but to fall back to a generic implementation for ARMv4. The platform is detected at run-time, after all. Indeed, this isn't terribly different from having a JIT that emits code for a specific architecture.

(In reply to comment #8) > (In reply to comment #4) > > #define ARM_ARCH_AT_LEAST(wanted) ((wanted <= __ARM_ARCH__) || (ARM_ARCH >= > > wanted)) > > Sadly, I'm don't think all ARM compilers reliably define these symbols... see > njcpudetect.h for the gyrations we go thru. (Debian GCC may always do so, but > if we are going to add ARMv4 support back, in let's do it robustly.) Actually, it appears not to be set on debian gcc either, which is weird because I thought it did ; which may be related to the fact that the original code was using it. Maybe only the android ndk provided gcc support it. (In reply to comment #9) > Ah, that makes sense to me now, though I'd go with something like ".arch > armv5t" as it's semantically more sensible. And it works. That's definitely better. > It feels a bit wrong to put target-specifying directives in inline assembly; I > don't know how the assembler handles .cpu and .arch directives in the middle of > assembly input. However, I can't think of a better way to achieve the effect > you're after, so your solution appears best. Apparently, gnu assembler handles it fine. It is already ifdef'ed on __GNUC__, so it should be fine. For extra caution, it could be ifdef'ed on __ARM_ARCH__ == 4 (or equivalent). (...) > Hmm. Anyway, I'll let Mozilla comment on this. Note that jstracer.cpp assumes armv4 by default.

(In reply to comment #10) > (In reply to comment #9) > > Ah, that makes sense to me now, though I'd go with something like ".arch > > armv5t" as it's semantically more sensible. > > And it works. That's definitely better. It actually doesn't work with the android ndk, where only at least ".arch armv7" does the right thing.

I'm going to test this patch more thouroughly, but feedback from Mozilla would be appreciated (cf. previous comments by Jacob) For trunk, I think I'll file other bugs and make the present bug depend on them: one for ARM_ARCH_AT_LEAST, which I think is nice to have even if armv4t is not considered, and another one for clz on android.

Hi Mike, I'm no expert, but aren't you missing __ARM_ARCH_4T__ in your patch? Thanks

(In reply to comment #13) > I'm no expert, but aren't you missing __ARM_ARCH_4T__ in your patch? You are right. I only figured recently, because I hadn't really tested the patch until recently. I'd still be interested in feedbaxk, though I should be able to split as per comment 12 quite soon.

This patch applies on top of the three other patches from the bug dependencies. I tested it successfully on nanojit-central.

Comment on attachment 464856 [details] [diff] [review] ARMv4T support for nanojit It looks sound, and I _think_ all the ARMv5-specific comments and things have been covered. I couldn't see any others with grep, at least. Slight nit: Some of the code paths in CountLeadingZeroes are pretty confusing. Is there a sensible way to split it? > I tested it successfully on nanojit-central. This also wants testing on tracemonkey and tamarin-redux.

Main differences with the previous version: - An additional comment in nanojit/NativeARM.h that should have been part of bug 585604. I could land that separately if necessary. - A comment change in nanojit/njconfig.h - Assembler::CountLeadingZeroes made more readable - A change to lirasm/lirasm.cpp to accept --arm-arch 4. (BTW, when landing, does that need to be separated out, as I see tamarin-redux doesn't have lirasm) Tested on an armv5t host forcing both armv4t and armv5t code generation on nanojit-central, tamarin-redux and tracemonkey.

Comment on attachment 469862 [details] [diff] [review] ARMv4T support for nanojit (In reply to comment #17) > - A change to lirasm/lirasm.cpp to accept --arm-arch 4. (BTW, when landing, > does that need to be separated out, as I see tamarin-redux doesn't have lirasm) All the changes you made should be applied only to nanojit-central, so you don't need to separate anything out. The changes will be pulled into tamarin-redux next time someone from Adobe runs their import script. I never actually noticed that tamarin-redux doesn't have lirasm, but the import script will surely filter out changes that don't apply.

Landed on nanojit-central http://hg.mozilla.org/projects/nanojit-central/rev/6b09fdb0cbc6