Closed
Bug 900756
Opened 11 years ago
Closed 11 years ago
Ionmonkey (ARM): add float32 support
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: dougc, Assigned: jonco)
References
(Blocks 1 open bug)
Details
(Whiteboard: [games])
Attachments
(3 files, 3 obsolete files)
36.31 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
12.27 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
45.11 KB,
patch
|
Details | Diff | Splinter Review |
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•11 years ago
|
Assignee: general → jcoppeard
Assignee | ||
Comment 1•11 years ago
|
||
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•11 years ago
|
||
Code generator and lowering support.
Attachment #796710 -
Flags: review?(mrosenberg)
Comment 3•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #796710 -
Flags: review?(mrosenberg) → review+
Assignee | ||
Comment 4•11 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•11 years ago
|
||
Updated patch following review comments.
Attachment #796708 -
Attachment is obsolete: true
Attachment #800658 -
Flags: review+
Assignee | ||
Comment 6•11 years ago
|
||
Update patch following review comments
Attachment #796710 -
Attachment is obsolete: true
Attachment #800659 -
Flags: review+
Reporter | ||
Comment 7•11 years ago
|
||
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•11 years ago
|
||
Attachment #804894 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e38bff7fe9c0
Comment 10•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e38bff7fe9c0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Reporter | ||
Updated•11 years ago
|
Whiteboard: [games]
Updated•11 years ago
|
Blocks: gecko-games
You need to log in
before you can comment on or make changes to this bug.
Description
•