586224 - Use clz on android even for armv5 target

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Patch

The android gcc compiler refuses to compile 'clz' for armv5, even though it is a legal instruction there, but it can still be tricked into compiling it.

This was already discussed in bug 552624

Comment 1

[Jacob Bramley [:jbramley]]

Comment on attachment 464742 [details] [diff] [review]
Patch

>-#elif defined(__GNUC__) && !(defined(ANDROID) && __ARM_ARCH__ <= 5)
>+#elif defined(__GNUC__) && (NJ_COMPILER_ARM_ARCH >= 5)

Ooh, actually the original code was broken here. Thanks for the fix!

----

A couple of tweaks:

>     // GCC can use inline assembler to insert a CLZ instruction.
>     __asm (
>+#if defined(ANDROID) && (NJ_COMPILER_ARM_ARCH <= 5)

I think this should be "NJ_COMPILER_ARM_ARCH < 7" (or "<= 6"). An Android+ARMv6 build will be broken with your current patch.

>+    // On Android gcc compiler, the clz instruction is not supported with a
>+    // target smaller than armv7, despite it being legal for armv5+.
>+        "   .arch armv7\n"

ARMv7-M only supports Thumb, so things might get confused as ".arch armv7" is supposed to represent a common set of all profiles. I think ".arch armv7-a" would be better.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Updated patch

Updated patch with your comments. Where should that land? nanojit-central?

Comment 3

[Mike Hommey [:glandium]]

http://hg.mozilla.org/projects/nanojit-central/rev/5a56d9f0b2d6

Comment 4

[Mike Hommey [:glandium]]

Reopening, as I've been told nanojit bugs get closed when landing on m-c.

Comment 5

[Edwin Smith]

merged into tamarin-redux
http://hg.mozilla.org/tamarin-redux/rev/c97ccde53276

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/4924f4e8d88f