1171649 - ARM JIT fixes for iOS

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[(not currently active) Ted Mielczarek]

Aside from bug 977805, the ARM JIT backend needs a few changes to build and work on iOS. This is mostly simple build fixes--changing places that assume OS X == x86 or that ARM == ELF, but there are a couple of other changes too:
* r9 is volatile per https://developer.apple.com/library/ios/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARMv6FunctionCallingConventions.html#//apple_ref/doc/uid/TP40009021-SW1
* irregexp needs to use more stack alignment
* Apple's toolchain doesn't provide __aeabi_idivmod and __aeabi_uidivmod, so I copied the assembly files from llvm's compiler-rt into the tree

Comment 1

[(not currently active) Ted Mielczarek]

Waldo asked on IRC if we could just copy files from compiler-rt like that. The license header in the files says they're dual-licensed under MIT and the University of Illinois Open Source License, so I don't see a problem:
http://llvm.org/klaus/compiler-rt/blob/eb2ecd0b49dcc15e4ffbf241e6fdc2e76e055efd/lib/arm/aeabi_idivmod.S

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: bug 1171649 - Implement arm/iOS support in JS JITs. r?jandem

bug 1171649 - Implement arm/iOS support in JS JITs. r?jandem

This patch includes some assembly files from llvm's compiler-rt to implement __aeabi_ividmod and __aeabi_uidivmod.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8616053 [details]
MozReview Request: bug 1171649 - Implement arm/iOS support in JS JITs. r?jandem

https://reviewboard.mozilla.org/r/10423/#review9749

::: js/src/asmjs/AsmJSSignalHandlers.cpp:337
(Diff revision 1)
> +#error Unsupported architecture

Nit: two spaces between '#' and 'error'

::: js/src/asmjs/AsmJSSignalHandlers.cpp:824
(Diff revision 1)
> +# error Unsupported architecture

And here.

::: js/src/asmjs/AsmJSSignalHandlers.cpp:881
(Diff revision 1)
> +# error Unsupported architecture

And here.

::: js/src/jit/ExecutableAllocator.h:426
(Diff revision 1)
> +#elif defined(JS_CODEGEN_ARM) && defined(__APPLE__)

And here for consistency.

::: js/src/moz.build:418
(Diff revision 1)
> +    if CONFIG['OS_ARCH'] == 'Darwin':

What about the ARM simulator build? Maybe make this an "elif" to avoid building it there?

::: js/src/jit/arm/Architecture-arm.cpp:253
(Diff revision 1)
> +        flags |= HWCAP_VFPv3 | HWCAP_VFPD32

Maybe good to double check we set HWCAP_ARMv7, HWCAP_VFP and HWCAP_USE_HARDFP_ABI on iOS.

::: js/src/irregexp/NativeRegExpMacroAssembler.cpp:129
(Diff revision 1)
> +    masm.movePtr(StackPointer, temp0);

Maybe use the iPhone #if + defined(JS_CODEGEN_ARM) (so we don't do this on ARM64) here and below?

I'd also add a brief explanation, something like:

// The stack is 4-byte aligned on iOS, force 8-byte alignment.

::: js/src/jit/ExecutableAllocator.h:60
(Diff revision 1)
> +#if defined(JS_CODEGEN_ARM) && defined(__APPLE__)

Use the iPhone #if here, to avoid potentially breaking the ARM simulator builds (x86 builds with JS_CODEGEN_ARM defined).

::: js/src/jit/arm/Architecture-arm.h:113
(Diff revision 1)
> +#if defined(__APPLE__)

This probably shouldn't be done for the simulator builds.

::: js/src/jit/assembly.h:3
(Diff revision 1)
> + *                     The LLVM Compiler Infrastructure

I think it'd be nice to move this into js/src/jit/llvm/ (or another name), to make it more clear it's imported..

Comment 4

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/10423/#review9749

> Maybe good to double check we set HWCAP_ARMv7, HWCAP_VFP and HWCAP_USE_HARDFP_ABI on iOS.

I checked, and `__ARM_ARCH_7A__` and `__VFP_FP__` are defined (and not `__SOFTFP__`), so the existing checks will work fine. For the latter I had to fix the check in Architecture-arm.h.

Comment 5

[Tom S [:evilpie]]

Do we need to add the "The LLVM Compiler Infrastructure" license to about:license?

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to Tom Schuster [:evilpie] from comment #5)
> Do we need to add the "The LLVM Compiler Infrastructure" license to
> about:license?

That is an excellent question. Gerv: this patch imports two assembly files into the tree from LLVM's compiler-rt project for use in ARM/iOS builds (which we have no current plans to ship, FWIW). See:
https://reviewboard.mozilla.org/r/10423/diff/1#5

Their license header says "This file is dual licensed under the MIT and the University of Illinois Open Source Licenses. See LICENSE.TXT for details.", where the LICENSE.TXT in question is:
http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/LICENSE.TXT?revision=232089&view=markup

Do we need to add this to about:license?

Comment 7

[Gervase Markham [:gerv]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> (In reply to Tom Schuster [:evilpie] from comment #5)
> That is an excellent question. Gerv: this patch imports two assembly files
> into the tree from LLVM's compiler-rt project for use in ARM/iOS builds
> (which we have no current plans to ship, FWIW). See:
> https://reviewboard.mozilla.org/r/10423/diff/1#5

If we aren't shipping it, we don't need to do anything, as long as we remember to do something when we do ship.

> Their license header says "This file is dual licensed under the MIT and the
> University of Illinois Open Source Licenses. 

Yeah. Why on earth? Those two licenses are near-identical in effect.

> See LICENSE.TXT for details.",
> where the LICENSE.TXT in question is:
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/LICENSE.
> TXT?revision=232089&view=markup
> 
> Do we need to add this to about:license?

See above. You could, or you could file a bug and make it depend on the "ship ARM/iOS" bug, and then ignore it.

If we did something, it would only require us to add one of the licenses. MIT is easier. We could call it the "LLVM license", even though that's slightly inaccurate.

Gerv

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8616053 [details]
MozReview Request: bug 1171649 - Implement arm/iOS support in JS JITs. r?jandem

bug 1171649 - Implement arm/iOS support in JS JITs. r?jandem

This patch includes some assembly files from llvm's compiler-rt to implement __aeabi_ividmod and __aeabi_uidivmod.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8616053 [details]
MozReview Request: bug 1171649 - Implement arm/iOS support in JS JITs. r?jandem

https://reviewboard.mozilla.org/r/10423/#review18043

Nice. r=me with nits below addressed.

::: js/src/irregexp/NativeRegExpMacroAssembler.cpp:380
(Diff revision 2)
> +#ifdef JS_CODEGEN_ARM

This should match the previous hunk:

#if defined(XP_IOS) && defined(JS_CODEGEN_ARM)

::: js/src/jit/ExecutableAllocator.h:419
(Diff revision 2)
> +      sys_icache_invalidate(code, size);

Nit: make this a 4 space indent instead of 2

::: js/src/jit/arm/Architecture-arm.h:126
(Diff revision 2)
> +#if !defined(__APPLE__)

To match the previous hunk and to avoid breaking the ARM simulator:

#if !defined(XP_IOS)

Comment 10

[(not currently active) Ted Mielczarek]

I punted the licensing stuff to bug 1207632.

Comment 11

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbceedb4370fbe729def191742ed1b5a4df34fa3
bug 1171649 - Implement arm/iOS support in JS JITs. r=jandem

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cbceedb4370f