802358 - IonMonkey: (ARM) Add support for hardfp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Marty Rosenberg [:mjrosenb]]

Looks like I fat-fingered enter there.  Anyhow.
IonMonkey is currently hardcoded to the softfp abi, which is what android uses.  Nearly every other ARM OS, however, runs hardfp, which notably passes double arguments in double registers (this does not matter much for code, since Values still get passed in pairs of registers).  I recently built a hardfp shell, and confirmed that we fail a small number of jit-tests.  
I believe the only place where we need to make changes is in callWithABI (possibly the VMCall infrastructure as well).
It should be a minimal amount of hacking, and until there is a strong request for this, I'll leave it for someone else who is interested in hacking on the ARM backend.

Comment 2

[Marty Rosenberg [:mjrosenb]]

Based on what I remember of implementing this the first time around, porting it over to hardfp should be pretty simple!

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #1)
> Looks like I fat-fingered enter there.  Anyhow.
> IonMonkey is currently hardcoded to the softfp abi, which is what android
> uses.  Nearly every other ARM OS, however, runs hardfp, which notably passes
> double arguments in double registers (this does not matter much for code,
> since Values still get passed in pairs of registers).  I recently built a
> hardfp shell, and confirmed that we fail a small number of jit-tests.  
> I believe the only place where we need to make changes is in callWithABI
> (possibly the VMCall infrastructure as well).

I guess, the main goal here are math functions which are called from the CodeGenerator with callWithABI.

No modifications would be needed for CallVM since VM function calls are not using any double value argument, the best might be to prevent it to happen by instrumenting VMFunction.h with compilation-failure case if a double is given as argument.

Comment 4

[Marty Rosenberg [:mjrosenb]]

Attachment: hardfp abi patch

After fighting with the toolchain a bit to get softfp builds by default on a hardfp machine, and needing to change the format of getArgReg to be more compatible with x64, I decided to just write this patch myself.

The only thing I am uncertain of at the moment is how to reliably detect the abi that we are compiling for.  I have not tried to build with clang yet.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #4)
> The only thing I am uncertain of at the moment is how to reliably detect the
> abi that we are compiling for.  I have not tried to build with clang yet.

GCC defines _ARM_PCS_VFP; unfortunately not all versions that support hardfp actually define the preprocessor symbol. :(  (I want to say 4.6 was the first to do both, but I'd have to go check.)  I think that's actually defined in the AAPCS, so clang ought to define it as well.

Comment 6

[Siarhei Siamashka]

(In reply to Marty Rosenberg [:mjrosenb] from comment #4)
> The only thing I am uncertain of at the moment is how to reliably detect the
> abi that we are compiling for.  I have not tried to build with clang yet.

Have a look at:
    https://github.com/mozilla/mozilla-central/blob/master/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp#L14
and
    http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45886

It was never fixed in the upstream FSF gcc 4.5, but all the relevant linux distros (ubuntu/linaro, gentoo, ...) nevertheless apply this patch. So if the toolchain is using hardfloat calling conventions, but does not define __ARM_PCS_VFP, it can be safely assumed to be broken.

Comment 7

[Jacob Bramley [:jbramley]]

Comment on attachment 688721 [details] [diff] [review]
hardfp abi patch

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

I mostly just made minor issues, comments and the like, but there are too many of them to justify an r+-with-changes.

::: js/src/ion/arm/Architecture-arm.h
@@ +8,5 @@
>  #ifndef jsion_architecture_arm_h__
>  #define jsion_architecture_arm_h__
>  
>  #include <limits.h>
> +// gcc appears to use ARM_PCS_VFP to denote that the target is a hard-float target.

s/ARM_PCS_VFP/__ARM_PCS_VFP/

::: js/src/ion/arm/Assembler-arm.h
@@ +1945,2 @@
>  static inline bool
> +#ifdef JS_CPU_ARM_HARDFP

The "#ifdef ..." needs to go before "static inline bool", otherwise you'll break the softfp build.

@@ +1962,5 @@
> +    return true;
> +}
> +
> +static inline uint32
> +GetIntArgStackDisp(uint32 *arg, uint32 *floatArgs)

Neither argument is modified here, so pass them by value.

Also, calling them usedIntArgs and usedFloatArgs (like the calling code) would be a good idea, since the code would be broken if they really are the total number of arguments.

@@ +1964,5 @@
> +
> +static inline uint32
> +GetIntArgStackDisp(uint32 *arg, uint32 *floatArgs)
> +{
> +

Extra whitespace.

@@ +1979,5 @@
> +    JS_ASSERT(*floatArg >= NumFloatArgRegs);
> +    uint32 argSlots = 0;
> +    if (*args > NumIntArgRegs) {
> +        argSlots = *args - NumIntArgRegs;
> +        *args = (*args + 1) & -2;

This seems to be accounting for the 8-byte alignment required by doubles in the calling convention. I think you need to put a comment in here because it looks weird, and it means that at the end, intArgs + floatArgs might not match the total number of arguments. Again, that's weird.

Alternately, consider passing a "padding" argument which tracks this unused space without affecting your argument counters. That way, "args" and "floatArg" could be passed by value.

Also, the naming could be improved, as I described for GetIntArgStackDisp.

@@ +1981,5 @@
> +    if (*args > NumIntArgRegs) {
> +        argSlots = *args - NumIntArgRegs;
> +        *args = (*args + 1) & -2;
> +    }
> +    uint32 floatSlots = *floatArg - NumFloatArgRegs;

I'd call this "doubleSlots" to be clear.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +2694,5 @@
> +        if (GetFloatArgReg(usedIntSlots_, usedFloatSlots_, &fr)) {
> +            enoughMemory_ = moveResolver_.addMove(from, MoveOperand(fr), Move::DOUBLE);
> +        } else {
> +            // If (and only if) the integer registers have started spilling, do we
> +            // need to take the double register's alignment into accoun

t.

@@ +2707,5 @@
> +        } else {
> +            uint32 disp = GetIntArgStackDisp(&usedIntSlots_, &usedFloatSlots_);
> +            enoughMemory_ = moveResolver_.addMove(from, MoveOperand(sp, disp), Move::GENERAL);
> +        }
> +            usedIntSlots_++;

Indentation.

@@ +2777,5 @@
>  MacroAssemblerARMCompat::callWithABI(void *fun, Result result)
>  {
>      JS_ASSERT(inCall_);
> +#ifdef JS_CPU_ARM_HARDFP
> +    uint32 stackAdjust = ((usedIntSlots_ > NumIntArgRegs) ? usedIntSlots_ - NumIntArgRegs : 0) * STACK_SLOT_SIZE;

This would be more concise, and probably clearer to read:
uint32 stackAdjust = Max(NumIntArgRegs, usedIntSlots_) - NumIntArgRegs +
                     Max(NumFloatArgRegs, usedFloatSlots_) - NumFloatArgRegs;
stackAdjust *= STACK_SLOT_SIZE;

@@ +2815,2 @@
>          // Move double from r0/r1 to ReturnFloatReg.
>          as_vxfer(r0, r1, ReturnFloatReg, CoreToFloat);

Does as_vxfer take arguments in a funny order? Otherwise, the as_vmov call is wrong.

::: js/src/jsval.h
@@ +249,5 @@
>      double asDouble;
>      void *asPtr;
>  } JSVAL_ALIGNMENT jsval_layout;
>  # elif JS_BITS_PER_WORD == 64
> +#error 64

Is this debug code?

@@ +273,5 @@
>      size_t asWord;
>      uintptr_t asUIntPtr;
>  } JSVAL_ALIGNMENT jsval_layout;
> +#else
> +#error WTF

Should these lines be here? If so, should they be indented to a level below the subsequent "# endif"?

::: media/webrtc/shared_libs.mk
@@ -27,5 @@
>    $(call EXPAND_LIBNAME_PATH,udp_transport,$(DEPTH)/media/webrtc/trunk/src/modules/modules_udp_transport) \
>    $(call EXPAND_LIBNAME_PATH,bitrate_controller,$(DEPTH)/media/webrtc/trunk/src/modules/modules_bitrate_controller) \
>    $(call EXPAND_LIBNAME_PATH,remote_bitrate_estimator,$(DEPTH)/media/webrtc/trunk/src/modules/modules_remote_bitrate_estimator) \
>    $(call EXPAND_LIBNAME_PATH,video_processing,$(DEPTH)/media/webrtc/trunk/src/modules/modules_video_processing) \
> -  $(call EXPAND_LIBNAME_PATH,video_processing_sse2,$(DEPTH)/media/webrtc/trunk/src/modules/modules_video_processing_sse2) \

I don't think you meant to remove that.

@@ -33,5 @@
>    $(call EXPAND_LIBNAME_PATH,audio_conference_mixer,$(DEPTH)/media/webrtc/trunk/src/modules/modules_audio_conference_mixer) \
>    $(call EXPAND_LIBNAME_PATH,audio_device,$(DEPTH)/media/webrtc/trunk/src/modules/modules_audio_device) \
>    $(call EXPAND_LIBNAME_PATH,audio_processing,$(DEPTH)/media/webrtc/trunk/src/modules/modules_audio_processing) \
>    $(call EXPAND_LIBNAME_PATH,aec,$(DEPTH)/media/webrtc/trunk/src/modules/modules_aec) \
> -  $(call EXPAND_LIBNAME_PATH,aec_sse2,$(DEPTH)/media/webrtc/trunk/src/modules/modules_aec_sse2) \

I don't think you meant to remove that.

@@ +55,5 @@
> +WEBRTC_LIBS += \
> +  $(call EXPAND_LIBNAME_PATH,aecm_neon,$(DEPTH)/media/webrtc/trunk/src/modules/modules_aecm_neon) \
> +  $(NULL)
> +endif
> +

I don't think you meant to add that.

::: media/webrtc/trunk/src/build/arm_neon.gypi
@@ +34,5 @@
>          '-flax-vector-conversions',
>        ],
> +      'cflags_mozilla': [
> +        '-mfpu=neon',
> +        '-mfloat-abi=softfp',

Do you rely on the default for other platforms?

Comment 8

[Marty Rosenberg [:mjrosenb]]

oh, all of the changes that were not in js were someone else's patch that I'd borrowed in order to get it building on arm in the first place.

Comment 9

[Marty Rosenberg [:mjrosenb]]

Attachment: hardfp abi patch v2

Think I fixed everything, but it's been a while since I fixed it, so I may have forgotten something.

Comment 10

[Jacob Bramley [:jbramley]]

Comment on attachment 689661 [details] [diff] [review]
hardfp abi patch v2

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

Yep, it looks good, except for one comment inconsistency.

::: js/src/ion/arm/Architecture-arm.h
@@ +198,5 @@
>  
>      static const uint32 WrapperMask = VolatileMask;
>  
>      // d0 is the ARM scratch float register.
> +    static const uint32 NonAllocatableMask = (1 << d1) | (1 << invalid_freg);

This no longer matches the comment. Is d1 now the scratch register?

Comment 11

[Marty Rosenberg [:mjrosenb]]

Landed: http://hg.mozilla.org/integration/mozilla-inbound/rev/eede2ac054bf

Comment 12

[(no longer active)]

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