Closed Bug 1431402 Opened 2 years ago Closed 2 years ago

Move int64-to-floating and floating-to-int64 conversion to MacroAssembler.h

Categories

(Core :: JavaScript Engine: JIT, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: lth, Assigned: lth)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Wasm has expanded the set of operations here, and there's no reason why these have to be both platform specific (in one case) and hidden in platform header files.

(Also I need to pave the way for ARM64.)
Floating point to int64/uint64.  By and large, this only promotes code from the platform masm to the common masm by moving code around and removing asMasm() calls that are now incorrect.  (Also I included stubs for ARM64 because that simplifies my job elsewhere.)
Attachment #8943604 - Flags: review?(nicolas.b.pierron)
Int64/uint64 to floating point.  Again, this mostly promotes code from the platform masm to the common masm.  I included ARM64 definitions because they are easy but I can stub them out if you prefer.
Attachment #8943605 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8943604 [details] [diff] [review]
bug1431402-fp-to-int64.patch

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

::: js/src/jit/arm64/MacroAssembler-arm64.cpp
@@ +885,5 @@
> +void
> +MacroAssembler::wasmTruncateDoubleToInt64(FloatRegister input, Register64 output, Label* oolEntry,
> +                                          Label* oolRejoin, FloatRegister tempDouble)
> +{
> +    MOZ_CRASH("NYI");

WasmBaseline will cause execution failures on arm64, is that expected? Wouldn't that break any harness?
Attachment #8943604 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Comment on attachment 8943604 [details] [diff] [review]
> bug1431402-fp-to-int64.patch
> 
> Review of attachment 8943604 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/arm64/MacroAssembler-arm64.cpp
> @@ +885,5 @@
> > +void
> > +MacroAssembler::wasmTruncateDoubleToInt64(FloatRegister input, Register64 output, Label* oolEntry,
> > +                                          Label* oolRejoin, FloatRegister tempDouble)
> > +{
> > +    MOZ_CRASH("NYI");
> 
> WasmBaseline will cause execution failures on arm64, is that expected?

Yes.

> Wouldn't that break any harness?

Wasm's currently completely disabled on ARM64.  I'm working on baseline support, and I have an implementation for this one ready to go as soon as I have enough code working to test it...
Comment on attachment 8943605 [details] [diff] [review]
bug1431402-int64-to-fp.patch

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

::: js/src/jit/mips64/MacroAssembler-mips64.cpp
@@ +2580,5 @@
> +void
> +MacroAssembler::convertUInt64ToDouble(Register64 src, FloatRegister dest, Register temp)
> +{
> +    MOZ_ASSERT(temp == Register::Invalid());
> +    convertUInt64ToDouble(src.reg, dest);

nit: MacroAssemblerSpecific::convertUInt64ToDouble or inline it here.

::: js/src/jit/mips64/MacroAssembler-mips64.h
@@ -984,5 @@
>  
> -    void convertInt64ToDouble(Register src, FloatRegister dest);
> -    void convertInt64ToFloat32(Register src, FloatRegister dest);
> -
> -    void convertUInt64ToDouble(Register src, FloatRegister dest);

nit: The declaration is removed, but the definition remains. (this is the 2 register variant, not the 3 register one of the generic MacroAssembler)

::: js/src/jit/x86/MacroAssembler-x86.cpp
@@ +1114,5 @@
> +// Convert floating point.
> +
> +// vpunpckldq requires 16-byte boundary for memory operand.
> +// See convertUInt64ToDouble for the details.
> +MOZ_ALIGNED_DECL(static const uint64_t, 16) TO_DOUBLE[4] = {

nit: double check with the check-masm target, otherwise move it else-where in this file.
Attachment #8943605 - Flags: review?(nicolas.b.pierron) → review+
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f58e75a8a20
Add floating-point-to-64bit-int conversion to MacroAssembler.h. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d87461d51a
Add 64bit-int-to-floating-point conversion to MacroAssembler.h. r=nbp
https://hg.mozilla.org/mozilla-central/rev/4f58e75a8a20
https://hg.mozilla.org/mozilla-central/rev/f9d87461d51a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.