Closed
Bug 676287
Opened 13 years ago
Closed 12 years ago
IonMonkey: inline some constant doubles in code
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(1 file, 1 obsolete file)
10.34 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
I am going to implement http://www.agner.org/optimize/optimizing_assembly.pdf 13.4.
Cool! You probably want to do this on top of bug 675395 which does the load-from-constant-pool thing. Our experience from JM was that eliminating memory traffic in building doubles was really important, but I don't know the tradeoff versus using a temporary register.
Assignee | ||
Comment 2•13 years ago
|
||
I am working on these also i have a few question: - i could possibly implement some algorithm that could detect doubles that would be possible to inline, some ideas for that? - i think i could manually also do inf, -inf and maybe nan (not sure if it's the right one) I am pretty sure i should organize this better.
Attachment #554496 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #554496 -
Flags: review? → review?(dvander)
Comment on attachment 554496 [details] [diff] [review] v0 Review of attachment 554496 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but is there a way to not duplicate these constants? Like having something in MacroAssembler-shared-x86: bool maybeInlineDouble(double d, const FloatRegister &dest) ...
Attachment #554496 -
Flags: review?(dvander)
Assignee | ||
Updated•13 years ago
|
Assignee: general → evilpies
Assignee | ||
Comment 4•12 years ago
|
||
Updated this patch \o/. So I closely followed Agner and looked at the generated code in gdb to make sure it didn't make a mistake in the assembler, but still review the assembler changes carefully please. As a further optimization for x86 we could optimize the case where the 32 bottom bits of the double are zero.
Attachment #554496 -
Attachment is obsolete: true
Attachment #633800 -
Flags: review?(dvander)
Comment on attachment 633800 [details] [diff] [review] v1 Review of attachment 633800 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/assembler/assembler/X86Assembler.h @@ +286,5 @@ > OP2_ANDPD_VpdWpd = 0x54, > OP2_XORPD_VpdWpd = 0x57, > OP2_MOVD_VdEd = 0x6E, > OP2_PSRLDQ_Vd = 0x73, > + OP2_PCMPEQW = 0x75, /* how does naming here work? */ nit: Remove this comment ::: js/src/ion/shared/MacroAssembler-x86-shared.h @@ +357,5 @@ > + default: > + return false; > + } > + return true; > + nit: extra newline (could just return true instead of using breaks)
Attachment #633800 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Looks like this might actually regress on sunspider.
Comment 7•12 years ago
|
||
Comment on attachment 633800 [details] [diff] [review] v1 Review of attachment 633800 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/assembler/assembler/X86Assembler.h @@ +675,5 @@ > #endif > > void fstp_m(int offset, RegisterID base) > { > + m_formatter.oneByteOp(OP_FPU6, FPU6_OP_FSTP, base, offset); tab
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/7fa6fdc6d1cf
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•