Last Comment Bug 676287 - IonMonkey: inline some constant doubles in code
: IonMonkey: inline some constant doubles in code
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: ---
Assigned To: Tom Schuster [:evilpie]
: Jason Orendorff [:jorendorff]
Depends on: 675395
  Show dependency treegraph
Reported: 2011-08-03 09:56 PDT by Tom Schuster [:evilpie]
Modified: 2012-06-27 13:07 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v0 (4.58 KB, patch)
2011-08-19 12:02 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
v1 (10.34 KB, patch)
2012-06-16 06:00 PDT, Tom Schuster [:evilpie]
dvander: review+
Details | Diff | Splinter Review

Description User image Tom Schuster [:evilpie] 2011-08-03 09:56:24 PDT
I am going to implement 13.4.
Comment 1 User image David Anderson [:dvander] 2011-08-03 11:05:06 PDT
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 User image Tom Schuster [:evilpie] 2011-08-19 12:02:20 PDT
Created attachment 554496 [details] [diff] [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.
Comment 3 User image David Anderson [:dvander] 2011-08-29 14:27:13 PDT
Comment on attachment 554496 [details] [diff] [review]

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 User image Tom Schuster [:evilpie] 2012-06-16 06:00:31 PDT
Created attachment 633800 [details] [diff] [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.
Comment 5 User image David Anderson [:dvander] 2012-06-18 15:42:31 PDT
Comment on attachment 633800 [details] [diff] [review]

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 User image Tom Schuster [:evilpie] 2012-06-24 09:58:56 PDT
Looks like this might actually regress on sunspider.
Comment 7 User image :Ms2ger (⌚ UTC+1/+2) 2012-06-24 11:04:42 PDT
Comment on attachment 633800 [details] [diff] [review]

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

Comment 8 User image Tom Schuster [:evilpie] 2012-06-27 13:07:33 PDT

Note You need to log in before you can comment on or make changes to this bug.