910796 - Various micro-optimizations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

The following is a serious of miscellaneous micro-optimizations.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: micro-pow-half.patch

Comment 2

[Dan Gohman [:sunfish]]

Attachment: micro-sqrt.patch

Comment 3

[Dan Gohman [:sunfish]]

Attachment: micro-abs.patch

Comment 4

[Dan Gohman [:sunfish]]

Attachment: micro-inc64.patch

Comment 5

[Dan Gohman [:sunfish]]

Attachment: micro-test-negative-zero.patch

Comment 6

[Sean Stangl [:sstangl]]

Comment on attachment 797352 [details] [diff] [review]
micro-pow-half.patch

Review of attachment 797352 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 7

[Dan Gohman [:sunfish]]

Attachment: micro-clamp-int32-to-uint8.patch

Comment 8

[Dan Gohman [:sunfish]]

Attachment: micro-args-rectifier.patch

Comment 9

[Dan Gohman [:sunfish]]

Attachment: micro-fold-load-branch.patch

Comment 10

[Dan Gohman [:sunfish]]

Attachment: micro-load-baseline-frame-ptr.patch

Comment 11

[Dan Gohman [:sunfish]]

Attachment: micro-compare-int32.patch

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 797362 [details] [diff] [review]
micro-args-rectifier.patch

Review of attachment 797362 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/x64/Trampoline-x64.cpp
@@ +361,2 @@
>  
> +        masm.addl(Imm32(1), r8);

nit: move this line out-side the current scope as it is not repeated in the loop.

::: js/src/jit/x86/Trampoline-x86.cpp
@@ +357,2 @@
>  
> +        masm.addl(Imm32(1), esi);

nit: same here.

Comment 13

[Sean Stangl [:sstangl]]

The two other patches up for review from me replace instructions with loads from memory. Are the replacements actually better?

Comment 14

[Sean Stangl [:sstangl]]

Comment on attachment 797362 [details] [diff] [review]
micro-args-rectifier.patch

Review of attachment 797362 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nicolas' suggestions.

Comment 15

[Dan Gohman [:sunfish]]

(In reply to Sean Stangl [:sstangl] from comment #13)
> The two other patches up for review from me replace instructions with loads
> from memory. Are the replacements actually better?

Assuming that the loads don't stall for some reason, both PowHalfD and AbsD are faster this way, at least on micro-benchmarks on my Sandy Bridge. According to Agner, the movsd load has latency 3 (assuming L1 cache hit etc.), which is the same as subsd (as seen in AbsD) and cvtss2sd (as seen in PowHalfD).

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 797354 [details] [diff] [review]
micro-sqrt.patch

Review of attachment 797354 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

We could do the same for AbsD on ARM to give regalloc a bit more freedom. But we'd have to move the code from Lowering to Lowering-arm/Lowering-x86-shared.

Comment 17

[Dan Gohman [:sunfish]]

(In reply to Nicolas B. Pierron [:nbp] from comment #12)
> Comment on attachment 797362 [details] [diff] [review]
> micro-args-rectifier.patch
> 
> Review of attachment 797362 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/x64/Trampoline-x64.cpp
> @@ +361,2 @@
> >  
> > +        masm.addl(Imm32(1), r8);
> 
> nit: move this line out-side the current scope as it is not repeated in the
> loop.

Ok.

> ::: js/src/jit/x86/Trampoline-x86.cpp
> @@ +357,2 @@
> >  
> > +        masm.addl(Imm32(1), esi);
> 
> nit: same here.

Ok.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aceffa2fd260
https://hg.mozilla.org/integration/mozilla-inbound/rev/414d6a9f1892
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac57b836d4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d436b68c7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd740f4f19c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3812728657bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/77e51c35f391
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea06b1ab196f

I didn't check in micro-inc64.patch. The performance impact of that change is less clear than I initially thought.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aceffa2fd260
https://hg.mozilla.org/mozilla-central/rev/414d6a9f1892
https://hg.mozilla.org/mozilla-central/rev/6ac57b836d4a
https://hg.mozilla.org/mozilla-central/rev/e2d436b68c7c
https://hg.mozilla.org/mozilla-central/rev/3dd740f4f19c
https://hg.mozilla.org/mozilla-central/rev/3812728657bd
https://hg.mozilla.org/mozilla-central/rev/77e51c35f391
https://hg.mozilla.org/mozilla-central/rev/ea06b1ab196f

Comment 19

[Dan Gohman [:sunfish]]

Here's the clampIntToUint8 patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d9f3a2e90f4

I'm dropping micro-inc64.patch.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d9f3a2e90f4