Various micro-optimizations

RESOLVED FIXED in mozilla26

Status

()

enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

Trunk
mozilla26
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments)

The following is a serious of miscellaneous micro-optimizations.
Attachment #797352 - Flags: review?(sstangl)
Attachment #797354 - Flags: review?(jdemooij)
Attachment #797355 - Flags: review?(sstangl)
Attachment #797356 - Flags: review?(bhackett1024)
Attachment #797358 - Flags: review?(sstangl)
Comment on attachment 797352 [details] [diff] [review]
micro-pow-half.patch

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

Nice.
Attachment #797352 - Flags: review?(sstangl) → review+
Attachment #797360 - Flags: review?(mrosenberg)
Attachment #797362 - Flags: review?(sstangl)
Attachment #797356 - Flags: review?(bhackett1024) → review+
Attachment #797365 - Flags: review?(kvijayan)
Attachment #797366 - Flags: review?(jdemooij)
Attachment #797367 - Flags: review?(evilpies)
Attachment #797367 - Flags: review?(evilpies) → review+
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.
Attachment #797362 - Flags: feedback+
The two other patches up for review from me replace instructions with loads from memory. Are the replacements actually better?
Attachment #797365 - Flags: review?(kvijayan) → review+
Comment on attachment 797362 [details] [diff] [review]
micro-args-rectifier.patch

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

r+ with nicolas' suggestions.
Attachment #797362 - Flags: review?(sstangl) → review+
(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 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.
Attachment #797354 - Flags: review?(jdemooij) → review+
Attachment #797366 - Flags: review?(jdemooij) → review+
Attachment #797355 - Flags: review?(sstangl) → review+
Attachment #797358 - Flags: review?(sstangl) → review+
(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.
Whiteboard: [leave open]
Attachment #797360 - Flags: review?(mrosenberg) → review+
Here's the clampIntToUint8 patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d9f3a2e90f4

I'm dropping micro-inc64.patch.
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/3d9f3a2e90f4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.