Closed Bug 676287 Opened 13 years ago Closed 12 years ago

IonMonkey: inline some constant doubles in code

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 675395
Attached patch v0 (obsolete) — Splinter Review
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?
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: general → evilpies
Attached patch v1Splinter Review
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+
Looks like this might actually regress on sunspider.
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
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.