676287 - IonMonkey: inline some constant doubles in code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S. (please needinfo tschuster)]

I am going to implement http://www.agner.org/optimize/optimizing_assembly.pdf 13.4.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: v0

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.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

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) ...

Comment 4

[Tom S. (please needinfo tschuster)]

Attachment: v1

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.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

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)

Comment 6

[Tom S. (please needinfo tschuster)]

Looks like this might actually regress on sunspider.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 8

[Tom S. (please needinfo tschuster)]

https://hg.mozilla.org/projects/ionmonkey/rev/7fa6fdc6d1cf