1051860 - SIMD x86-x64: Optimize MSimdValueX4

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1025475#c16 and https://bugzilla.mozilla.org/show_bug.cgi?id=1025475#c19 for more details.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Optimize SimdValueX4 codegen for float32x4 with unpcklps

This unpcklps trick is really magic.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Optimize SimdValueX4 codegen for int32x4 if SSE4 is available

And pinsrd was actually already implemented for unboxing double values in
x86... Nice!

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Optimize Float32x4 Codegen With This One Weird Trick

So it appears this first patch was really not regalloc friendly, in particular it clobbered two inputs without any shame. We need two temporaries (for r0 and r1), so by reusing r0's register for the output, we have one; then we need only one copy. This patch splits lowering for int32x4 and float32x4 (note that there is no LOpcodes-x86-shared.h and so the opcodes have to be repeated in each of the x64/LOpcodes-x64.h and x86 files).

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Optimize int32x4 codegen when SSE4.1 is available

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Optimize int32x4 codegen when SSE4.1 is available

forgot to qref

Comment 6

[Dan Gohman [:sunfish]]

Comment on attachment 8487061 [details] [diff] [review]
1. Optimize Float32x4 Codegen With This One Weird Trick

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

One Weird Trick that CPUs, um, actually like.

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2150,5 @@
> +    FloatRegister r3 = ToFloatRegister(ins->getOperand(3));
> +
> +    masm.unpcklps(r3, r1);
> +    masm.unpcklps(r2, r0);
> +    masm.unpcklps(r1, r0);

Comment 7

[Dan Gohman [:sunfish]]

Comment on attachment 8487063 [details] [diff] [review]
2. Optimize int32x4 codegen when SSE4.1 is available

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

Groovy. We could optimize the non-SSE4.1 case more, but not *that* much more, and it'd require temporary registers, so it's not worth worrying about for now.

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2127,5 @@
> +    FloatRegister output = ToFloatRegister(ins->output());
> +    if (AssemblerX86Shared::HasSSE41()) {
> +        for (size_t i = 0; i < 4; ++i) {
> +            Register r = ToRegister(ins->getOperand(i));
> +            masm.pinsrd(i, r, output);

You want movd for the first element, and pinsrd for the rest. movd clears the entire register, clearing dependencies on earlier instructions.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks!

Out of curiosity, how could we optimize the non SSE4.1 case more? Also, it looks your review comments of the first patch may have disappeared somehow (or there is just an empty comment that didn't get deleted). Please let me know if you had any remarks about the patch, I'll address them in a follow-up patch if needed.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/179193fbcccd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc38353cfa4

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/179193fbcccd
https://hg.mozilla.org/mozilla-central/rev/6cc38353cfa4

Comment 10

[Dan Gohman [:sunfish]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> Thanks!
> 
> Out of curiosity, how could we optimize the non SSE4.1 case more?

movd all the integers into xmm registers and then do the shuffles like in the float32 case to pack them together. Doing a shuffle after the first 2 movd can keep it to 2 temporaries:

  movd        %(W), %xmm0
  movd        %(Y), %xmm1
  punpckldq   %xmm0, %xmm1
  movd        %(Z), %xmm2
  movd        %(X), %xmm0
  punpckldq   %xmm2, %xmm0
  punpckldq   %xmm1, %xmm0

> Also, it
> looks your review comments of the first patch may have disappeared somehow
> (or there is just an empty comment that didn't get deleted). Please let me
> know if you had any remarks about the patch, I'll address them in a
> follow-up patch if needed.

I had posted a unicode emoji character celebrating the triple-unpcklps sequence. It looks like bugzilla swallowed it. The patch is fine :-).