Ionmonkey (ARM): add float32 support

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dougc, Assigned: jonco)

Tracking

(Blocks 1 bug)

unspecified
mozilla27
ARM
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [games])

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

6 years ago
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.
Assignee

Updated

6 years ago
Assignee: general → jcoppeard
Assignee

Comment 1

6 years ago
Posted patch float32-ARM-1 (obsolete) — Splinter Review
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.
Attachment #796708 - Flags: review?(mrosenberg)
Assignee

Comment 2

6 years ago
Posted patch float32-ARM-2 (obsolete) — Splinter Review
Code generator and lowering support.
Attachment #796710 - Flags: review?(mrosenberg)
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.
Attachment #796708 - Flags: review?(mrosenberg) → review+
Attachment #796710 - Flags: review?(mrosenberg) → review+
Assignee

Comment 4

6 years ago
(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.
Assignee

Comment 5

6 years ago
Posted patch float32-ARM-1Splinter Review
Updated patch following review comments.
Attachment #796708 - Attachment is obsolete: true
Attachment #800658 - Flags: review+
Assignee

Comment 6

6 years ago
Posted patch float32-ARM-2Splinter Review
Update patch following review comments
Attachment #796710 - Attachment is obsolete: true
Attachment #800659 - Flags: review+
Reporter

Comment 7

6 years ago
Posted patch Combined rebased patch. (obsolete) — Splinter Review
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).
Reporter

Comment 8

6 years ago
Attachment #804894 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e38bff7fe9c0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Reporter

Updated

6 years ago
Depends on: 918206
Reporter

Updated

6 years ago
Whiteboard: [games]
You need to log in before you can comment on or make changes to this bug.