ARM64: Add build support

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sstangl, Assigned: sstangl)

Tracking

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 8621269 [details] [diff] [review]
0001-Add-ARM64-build-support.patch

This patch includes the configure.in and moz.build changes necessary to get the simulator building on ARM64. This does not include the ability to build on ARM64 hardware.
Attachment #8621269 - Flags: review?(mh+mozilla)
Comment on attachment 8621269 [details] [diff] [review]
0001-Add-ARM64-build-support.patch

Review of attachment 8621269 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/configure.in
@@ +2106,5 @@
>      AC_DEFINE(JS_CPU_ARM)
>      ;;
> +aarch64*-*)
> +    ENABLE_ION=1
> +    AC_DEFINE(JS_CPU_ARM64)

This contradicts comment 0's "This does not include the ability to build on ARM64 hardware."

@@ +3129,5 @@
> +dnl JS_CODEGEN_foo is defined if JS_CPU_foo is defined.
> +MOZ_ARG_ENABLE_BOOL(arm64-simulator,
> +[  --enable-arm64-simulator Enable ARM64 simulator for JIT code],
> +    JS_ARM64_SIMULATOR=1,
> +    JS_ARM64_SIMULATOR= )

This needs to conflict with --enable-arm-simulator and --enable-mips-simulator... and then you realize that it's complicated and going to get worse for each additional one. Since we only really support one at a time, we should just change the option to be:
--enable-simulator=arch.

@@ +3171,5 @@
>      AC_DEFINE(JS_CODEGEN_ARM)
>      JS_CODEGEN_ARM=1
> +elif test "$CPU_ARCH" = "aarch64"; then
> +    AC_DEFINE(JS_CODEGEN_ARM64)
> +    JS_CODEGEN_ARM64=1

This contradicts comment 0's "This does not include the ability to build on ARM64 hardware."
Attachment #8621269 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 2

4 years ago
Created attachment 8621879 [details] [diff] [review]
Patch, Version 2

This patch is the previous patch, minus the aarch64 hardware bits that snuck in, plus changing the simulator configuration syntax to --enable-simulator=(arm, arm64, mips).

A number of places in the codebase contain code that looks like the following:
> #if defined(JS_ARM_SIMULATOR) || defined(JS_ARM64_SIMULATOR) || defined(JS_MIPS_SIMULATOR)

which suffers similar blowup. So this patch adds a JS_SIMULATOR for just checking that some simulator is present.

Additionally, since I was in the area, I changed the defines to match JS_CODEGEN_foo and JS_CPU_foo: JS_foo_SIMULATOR becomes JS_SIMULATOR_foo.
Attachment #8621269 - Attachment is obsolete: true
Attachment #8621879 - Flags: review?(mh+mozilla)
Comment on attachment 8621879 [details] [diff] [review]
Patch, Version 2

Review of attachment 8621879 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/AsmJSModule.cpp
@@ +661,4 @@
>  static void*
>  RedirectCall(void* fun, ABIFunctionType type)
>  {
> +#ifdef JS_SIMULATOR

Just to be sure, adding arm64 here is intended, right? (same question for other similar changes)

::: js/src/configure.in
@@ -971,5 @@
>      CPU_ARCH="mips"
>      ;;
>  
> -aarch64*)
> -    CPU_ARCH=aarch64

Please don't remove this, it has no effect on the rest, but is useful regardless of JIT support.

@@ +3108,5 @@
> +if test -n "$JS_SIMULATOR"; then
> +    case "$JS_SIMULATOR" in
> +        arm) ;;
> +        arm64) ;;
> +        mips) ;;

you can say arm|arm64|mips) ;;

you should also add:
  no)
    JS_SIMULATOR=
    ;;

(you get "no" if you pass --disable-simulator)

@@ +3126,2 @@
>      AC_DEFINE(JS_CODEGEN_ARM)
> +    JS_SIMULATOR=1

You don't actually need to set JS_SIMULATOR to 1. The value it has when you reach here (and the other ones below) is just fine.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +3758,4 @@
>      passedArgs_ = 0;
>      passedArgTypes_ = 0;
>      usedIntSlots_ = 0;
> +#if defined(JS_CODEGEN_ARM_HARDFP) || defined(JS_SIMULATOR_ARM)

shouldn't simulator=arm just enable CODEGEN_ARM_HARDFP, like it does enable CODEGEN_ARM ?
Attachment #8621879 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 4

4 years ago
Setting needinfo on Gary's request:

This bug changes the flag --enable-arm-simulator to --enable-simulator=arm.
After this lands, --enable-arm-simulator will surprisingly do an x86 build.
Flags: needinfo?(gary)
Flags: needinfo?(choller)
(In reply to Sean Stangl [:sstangl] from comment #4)
> Setting needinfo on Gary's request:
> 
> This bug changes the flag --enable-arm-simulator to --enable-simulator=arm.
> After this lands, --enable-arm-simulator will surprisingly do an x86 build.

If that's a problem, you could add back dummy --enable-arm-simulator and --enable-mips-simulator arguments that fail the build indicating the change.
So new values are going to be:

--enable-simulator=arm
--enable-simulator=arm64
--enable-simulator=mips

replacing --enable-arm-simulator.
https://hg.mozilla.org/mozilla-central/rev/25e99bc12482
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Harness is fixed in fuzzing rev 20bfe49788bb. Thanks for the headsup!
Flags: needinfo?(gary)
Flags: needinfo?(choller)
You need to log in before you can comment on or make changes to this bug.