1069956 - SIMD x86-x64 backend: implement from / fromBits and add them to Odin

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

int32x4.fromFloat32x4(float32x4 x) / float32x4.fromInt32x4(x) create a new vector with values being *converted* (~static_cast).

int32x4.fromFloat32x4Bits(float32x4 x) / float32x4.fromInt32x4Bits(x) create a new vector with bits being *reinterpreted* (~reinterpret_cast)

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1) SimdConvertFrom

- I wasn't really inspired for the naming. Maybe simply SimdConvert would be enough? The From suffix refers to the name in the spec / polyfill, but that could actually be removed.
- the only issue with the spec is with int32x4.fromFloat32x4(x). This function is supposed to map to cvtps2dq or cvttps2dq (i've chosen the latter for consistency with convertFloat32ToInt32()), which return the undefined int32 value (0x80000000) for float values that are bigger than an int32. There is a big comment and a link to bug 1068028 in the code, so that should be enough to catch up on the spec later.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2) SimdConvertFrom, Support in Odin

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3) SimdCast

This one doesn't even generate code, how nice is that :)

Simply using redefine() seems to be enough from my testing in Odin (even with a use of the cast operand afterwards), but will it always be safe in all contexts?

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4) SimdCast: support in Odin

Comment 5

[Dan Gohman [:sunfish]]

Comment on attachment 8493743 [details] [diff] [review]
1) SimdConvertFrom

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

::: js/src/jit/MIR.h
@@ +1361,5 @@
>      }
>  };
>  
> +// Converts all lanes of a given vector into the type of another vector
> +class MSimdConvertFrom : public MUnaryInstruction

How about just MSimdConvert?

@@ +1385,5 @@
> +    }
> +    bool congruentTo(const MDefinition *ins) const {
> +        return congruentIfOperandsEqual(ins);
> +    }
> +};

I haven't been consistently asking for it, but we could start putting ALLOW_CLONE declarations in operators like this. ALLOW_CLONE enables loop unrolling, and maybe eventually jump threading and loop rotation.

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +2185,5 @@
> +    FloatRegister in = ToFloatRegister(ins->input());
> +    FloatRegister out = ToFloatRegister(ins->output());
> +    masm.convertFloat32x4ToInt32x4(in, out);
> +    return true;
> +}

It seems likely that these operations will be implementable in the macro-assembler for any sane target, so can we move these two CodeGenerator functions into platform-independent CodeGenerator.cpp?

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +468,5 @@
>      }
>  
> +    void convertFloat32x4ToInt32x4(FloatRegister src, FloatRegister dest) {
> +        // TODO: Note that if the conversion failed (because the converted
> +        // result is larger than the maximum signed int32), this will return

... or less than the least signed int32, or NaN, for completeness.

Comment 6

[Dan Gohman [:sunfish]]

Comment on attachment 8493746 [details] [diff] [review]
3) SimdCast

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

::: js/src/jit/Lowering.cpp
@@ +3740,5 @@
>  bool
> +LIRGenerator::visitSimdCast(MSimdCast *ins)
> +{
> +    MOZ_ASSERT(IsSimdType(ins->type()) && IsSimdType(ins->input()->type()));
> +    return redefine(ins, ins->input());

The key requirement for redefine is that the input and output are in the same register class. On all targets we can reasonably anticipate today, this is true, so we should be ok here.

::: js/src/jit/MIR.h
@@ +1388,5 @@
>      }
>  };
>  
> +// Casts bits of a vector input to another SIMD type (doesn't generate code).
> +class MSimdCast : public MUnaryInstruction

Let's rename this to MSimdReinterpretCast to avoid ambiguity.

@@ +1412,5 @@
> +    }
> +    bool congruentTo(const MDefinition *ins) const {
> +        return congruentIfOperandsEqual(ins);
> +    }
> +};

This class can also get an ALLOW_CLONE.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8493749 [details] [diff] [review]
4) SimdCast: support in Odin

Perfect

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Addressed all nits, except for moving the codegen method to the shared codegen: a lot of other methods are in the same situation, so I'd really prefer have the ARM macroassembler methods implemented before moving them to shared codegen.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1926709eaf90
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fd0ef779e8a1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c965698b314e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e2c803c2aeec

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1926709eaf90
https://hg.mozilla.org/mozilla-central/rev/fd0ef779e8a1
https://hg.mozilla.org/mozilla-central/rev/c965698b314e
https://hg.mozilla.org/mozilla-central/rev/e2c803c2aeec