900756 - Ionmonkey (ARM): add float32 support

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Douglas Crosher [:dougc]]

Bug 888109 adds general float32 support.  ARM support just requires a little backend support.  It might be simplest for now to just use half of a double register pair on the ARM rather than trying to pack float32s into all available registers.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: float32-ARM-1

This patch provides assembler / macro assembler support for float32 operations.

I added Float32Encoder for float32 immediate constants and added a bit in PoolHintData to signify whether the destination register is double or float32 for floating point constant loads.

Currently the patch adds float32 constants to the double pool, wasting 4 bytes each time.  Another option would be for them to have their own pool, but I don't know enough about how pools work to know whether this is worth it.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: float32-ARM-2

Code generator and lowering support.

Comment 3

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 796708 [details] [diff] [review]
float32-ARM-1

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

::: js/src/jit/arm/Assembler-arm.cpp
@@ +1781,5 @@
> +    /*
> +     * Insert floats into the double pool as they have the same limitations on
> +     * immediate offset.  This wastes 4 bytes padding per float.  An alternative
> +     * would be to have a separate pool for floats.
> +     */

quite sad. The Assembler Buffer re-write should fix this.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +131,5 @@
> +}
> +
> +void
> +MacroAssemblerARM::convertInt32ToFloat32(const Register &src, const FloatRegister &dest_) {
> +    // direct conversions aren't possible.

I assume that "direct conversions" means doing a int32 -> float32 conversion and doing the gpr -> vfp transfer in one instruction?

@@ +140,5 @@
> +}
> +
> +void
> +MacroAssemblerARM::convertInt32ToFloat32(const Address &src, FloatRegister dest) {
> +    ma_ldr(Operand(src), ScratchRegister);

You should be able to load the int32 directly into tho dest register, then do the int32 -> float32 conversion in that register.  You should probably use the scratch, since I've head that immediately overwriting a register is bad for perf.  Since the vfp's offsets are more limited, this method may not save any instructions, but it should at the very least save a synchronization point.  Now that I think about it, this can almost certainly be applied to convertInt32ToFloat64 (or whatever it is actually called)

@@ +1323,5 @@
> +{
> +    as_vadd(VFPRegister(dst).singleOverlay(), VFPRegister(src1).singleOverlay(),
> +            VFPRegister(src2).singleOverlay());
> +}
> +

*idly wonders if we can have these functions take VFPRegisters*

@@ +1436,5 @@
> +    VFPRegister vd = VFPRegister(dest).singleOverlay();
> +    uint32_t spun = *reinterpret_cast<uint32_t*>(&value);
> +    if (hasVFPv3()) {
> +        if (spun == 0) {
> +            // To zero a register, load 1.0, then execute dN <- dN - dN

minor nit: those should be sN

@@ +1443,5 @@
> +            as_vsub(vd, vd, vd, cc);
> +            return;
> +        }
> +
> +        VFPImm floatEnc = VFPImm::forFloat32(spun);

Fwiw, the set of encodings for float32 and float64 is identical, so you should be able to just cast the float value to a double, and call the preexisting routine.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Marty Rosenberg [:mjrosenb] from comment #3)

Thanks for the comments.

> > +void
> > +MacroAssemblerARM::convertInt32ToFloat32(const Register &src, const FloatRegister &dest_) {
> > +    // direct conversions aren't possible.
> 
> I assume that "direct conversions" means doing a int32 -> float32 conversion
> and doing the gpr -> vfp transfer in one instruction?

Yes, it seems you need to transfer the integer value to the vfp register first, and then convert it.

> > +MacroAssemblerARM::convertInt32ToFloat32(const Address &src, FloatRegister dest) {
> > +    ma_ldr(Operand(src), ScratchRegister);
> 
> You should be able to load the int32 directly into tho dest register, then
> do the int32 -> float32 conversion in that register.  You should probably
> use the scratch, since I've head that immediately overwriting a register is
> bad for perf.  Since the vfp's offsets are more limited, this method may not
> save any instructions, but it should at the very least save a
> synchronization point.  Now that I think about it, this can almost certainly
> be applied to convertInt32ToFloat64 (or whatever it is actually called)

Good idea, done.

> @@ +1323,5 @@
> > +{
> > +    as_vadd(VFPRegister(dst).singleOverlay(), VFPRegister(src1).singleOverlay(),
> > +            VFPRegister(src2).singleOverlay());
> > +}
> > +
> 
> *idly wonders if we can have these functions take VFPRegisters*

Sounds good, I might look at that as a followup bug.

> > +            // To zero a register, load 1.0, then execute dN <- dN - dN
> 
> minor nit: those should be sN

Fixed.

> @@ +1443,5 @@
> > +            as_vsub(vd, vd, vd, cc);
> > +            return;
> > +        }
> > +
> > +        VFPImm floatEnc = VFPImm::forFloat32(spun);
> 
> Fwiw, the set of encodings for float32 and float64 is identical, so you
> should be able to just cast the float value to a double, and call the
> preexisting routine.

I didn't realise that.  I'll remove the separate float32 encoder.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: float32-ARM-1

Updated patch following review comments.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: float32-ARM-2

Update patch following review comments

Comment 7

[Douglas Crosher [:dougc]]

Attachment: Combined rebased patch.

An attempt to rebase these patches.  Tested in combination with
the patches for bug 915495 the ARM backend passes the standard
jit tests (shall try --tbpl now).

Comment 8

[Douglas Crosher [:dougc]]

Attachment: Rebased combined patch

Comment 9

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e38bff7fe9c0

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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