Closed Bug 1171649 Opened 7 years ago Closed 7 years ago

ARM JIT fixes for iOS

Categories

(Core :: JavaScript Engine: JIT, defect)

ARM
iOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: ted, Assigned: ted)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
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
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.
Attachment #8616053 - Flags: review?(jdemooij)
Attachment #8616053 - Flags: review?(jdemooij)
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..
Blocks: 735728
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.
Do we need to add the "The LLVM Compiler Infrastructure" license to about:license?
OS: Unspecified → iOS
Hardware: Unspecified → ARM
(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?
Flags: needinfo?(gerv)
OS: iOS → Unspecified
Hardware: ARM → Unspecified
OS: Unspecified → iOS
Hardware: Unspecified → ARM
Depends on: 1205273
(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
Flags: needinfo?(gerv)
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.
Attachment #8616053 - Flags: review?(jdemooij)
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)
Attachment #8616053 - Flags: review?(jdemooij) → review+
Blocks: 1207632
I punted the licensing stuff to bug 1207632.
https://hg.mozilla.org/mozilla-central/rev/cbceedb4370f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.