Closed Bug 1069956 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(4 files)

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)
Blocks: 1068028
- 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.
Attachment #8493743 - Flags: review?(sunfish)
Attached patch 3) SimdCastSplinter Review
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?
Attachment #8493746 - Flags: review?(sunfish)
Attachment #8493749 - Flags: review?(luke)
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.
Attachment #8493743 - Flags: review?(sunfish) → review+
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.
Attachment #8493746 - Flags: review?(sunfish) → review+
Attachment #8493744 - Flags: review?(luke) → review+
Comment on attachment 8493749 [details] [diff] [review]
4) SimdCast: support in Odin

Perfect
Attachment #8493749 - Flags: review?(luke) → review+
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
You need to log in before you can comment on or make changes to this bug.