Closed Bug 1272934 Opened 6 years ago Closed 6 years ago

IonMonkey: MIPS: Implement WasmTruncateToInt32

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: hev, Assigned: hev)

Details

Attachments

(3 files, 1 obsolete file)

1. Implement Assembler::as_ctc1/as_cfc1.
2. Implement Assembler::as_truncld/as_truncls.
3. Implement WasmTruncateToInt32.
Assignee: nobody → r
Status: NEW → ASSIGNED
Attachment #8752534 - Flags: review?(arai.unmht)
Attachment #8752535 - Flags: review?(arai.unmht)
Attachment #8752536 - Flags: review?(bbouvier)
Comment on attachment 8752534 [details] [diff] [review]
Part 1 - Implement Assembler::as_ctc1/as_cfc1.

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

Looks good :)
Attachment #8752534 - Flags: review?(arai.unmht) → review+
Attachment #8752535 - Flags: review?(arai.unmht) → review+
Comment on attachment 8752536 [details] [diff] [review]
Part 3 - Implement WasmTruncateToInt32.

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

Thank you for the patch! My knowledge of MIPS isn't too strong, but from what I've read from the architecture manual, this won't handle a few edge cases. I think these errors should be caught by the test suite; have you tried running the wasm/spec.js test with this patch?

::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
@@ +1348,5 @@
> +    if (!ool->isUnsigned()) {
> +        if (toType == MIRType::Int32) {
> +            // MWasmTruncateToInt32
> +            if (fromType == MIRType::Double) {
> +                // we've used cvtwd. the only valid double values that can

nit: truncwd

@@ +1354,5 @@
> +                masm.loadConstantDouble(double(INT32_MIN) - 1.0, ScratchDoubleReg);
> +                masm.branchDouble(Assembler::DoubleLessThanOrEqual, input, ScratchDoubleReg, &fail);
> +
> +                masm.loadConstantDouble(double(INT32_MIN), ScratchDoubleReg);
> +                masm.branchDouble(Assembler::DoubleGreaterThan, input, ScratchDoubleReg, &fail);

I think that truncwd will also set the Invalid Operation flag for values in the [INT64_MAX; INT64_MAX + 1[ range, which also need special handling.

Also, for values in the ]INT32_MIN - 1; INT32_MIN] range, won't the output be the default result value, that is INT64_MAX?

@@ +1358,5 @@
> +                masm.branchDouble(Assembler::DoubleGreaterThan, input, ScratchDoubleReg, &fail);
> +            } else {
> +                MOZ_ASSERT(fromType == MIRType::Float32);
> +
> +                // We've used cvtws. Check that the input wasn't

nit: truncws

@@ +1362,5 @@
> +                // We've used cvtws. Check that the input wasn't
> +                // float(INT32_MIN), which is the only legimitate input that
> +                // would truncate to INT32_MIN.
> +                masm.loadConstantFloat32(float(INT32_MIN), ScratchFloat32Reg);
> +                masm.branchFloat(Assembler::DoubleNotEqual, input, ScratchFloat32Reg, &fail);

Same comment for values in [INT32_MAX; INT32_MAX + 1[.

::: js/src/jit/mips32/CodeGenerator-mips32.cpp
@@ +326,5 @@
> +        masm.moveFromDoubleHi(ScratchDoubleReg, output);
> +        masm.as_cfc1(ScratchRegister, Assembler::FCSR);
> +        masm.as_ext(ScratchRegister, ScratchRegister, 16, 1);
> +        masm.ma_or(output, ScratchRegister);
> +        masm.ma_b(output, Imm32(0), ool->entry(), Assembler::NotEqual);

If we try to do an unsigned conversion of INT32_MAX + 1, this will jump to the OOL entry because it's outside the range of truncld/truncls and InvalidOperation will be set in FCSR, right? If so, I think it will directly trap there, but it shouldn't. Am I missing something?

Reading the MIPS architecture manual, it seems that truncld/truncls accepts ranges from INT64_MIN to INT64_MAX on both platforms, which contradicts your comment above. If so, I think the code of this entire function can be in mips-shared.

Also, what about values in [INT64_MAX; INT64_MAX + 1[, which will trigger the invalid operation, but should actually properly truncate to INT64MAX? (and the equivalent for INT64_MIN)

@@ +328,5 @@
> +        masm.as_ext(ScratchRegister, ScratchRegister, 16, 1);
> +        masm.ma_or(output, ScratchRegister);
> +        masm.ma_b(output, Imm32(0), ool->entry(), Assembler::NotEqual);
> +
> +        masm.moveFromFloat32(ScratchDoubleReg, output);

Just to be sure here: do we use moveFromFloat32 just to load the low 32 bits here, while on mips64 we can use moveFromDouble because we've tested the high bits are all 0? If so, for the same reason, I think we could use moveFromDouble on both platforms?
Attachment #8752536 - Flags: review?(bbouvier)
(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> Comment on attachment 8752536 [details] [diff] [review]
> Part 3 - Implement WasmTruncateToInt32.
> 
> Review of attachment 8752536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the patch! My knowledge of MIPS isn't too strong, but from
> what I've read from the architecture manual, this won't handle a few edge
> cases. I think these errors should be caught by the test suite; have you
> tried running the wasm/spec.js test with this patch?

Yes, All jit-tests passed with --tbpl.

> 
> ::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
> @@ +1348,5 @@
> > +    if (!ool->isUnsigned()) {
> > +        if (toType == MIRType::Int32) {
> > +            // MWasmTruncateToInt32
> > +            if (fromType == MIRType::Double) {
> > +                // we've used cvtwd. the only valid double values that can
> 
> nit: truncwd
> 
> @@ +1354,5 @@
> > +                masm.loadConstantDouble(double(INT32_MIN) - 1.0, ScratchDoubleReg);
> > +                masm.branchDouble(Assembler::DoubleLessThanOrEqual, input, ScratchDoubleReg, &fail);
> > +
> > +                masm.loadConstantDouble(double(INT32_MIN), ScratchDoubleReg);
> > +                masm.branchDouble(Assembler::DoubleGreaterThan, input, ScratchDoubleReg, &fail);
> 
> I think that truncwd will also set the Invalid Operation flag for values in
> the [INT64_MAX; INT64_MAX + 1[ range, which also need special handling.
The truncwd/truncws actually set invalid bit when Infinity, NaN and outside the range [INT32_MIN; INT32_MAX + 1[. I'll fix comments.

> 
> Also, for values in the ]INT32_MIN - 1; INT32_MIN] range, won't the output
> be the default result value, that is INT64_MAX?
> 
> @@ +1358,5 @@
> > +                masm.branchDouble(Assembler::DoubleGreaterThan, input, ScratchDoubleReg, &fail);
> > +            } else {
> > +                MOZ_ASSERT(fromType == MIRType::Float32);
> > +
> > +                // We've used cvtws. Check that the input wasn't
> 
> nit: truncws
> 
> @@ +1362,5 @@
> > +                // We've used cvtws. Check that the input wasn't
> > +                // float(INT32_MIN), which is the only legimitate input that
> > +                // would truncate to INT32_MIN.
> > +                masm.loadConstantFloat32(float(INT32_MIN), ScratchFloat32Reg);
> > +                masm.branchFloat(Assembler::DoubleNotEqual, input, ScratchFloat32Reg, &fail);
> 
> Same comment for values in [INT32_MAX; INT32_MAX + 1[.
For Float32, direct jump to IntegerOverflow?

> 
> ::: js/src/jit/mips32/CodeGenerator-mips32.cpp
> @@ +326,5 @@
> > +        masm.moveFromDoubleHi(ScratchDoubleReg, output);
> > +        masm.as_cfc1(ScratchRegister, Assembler::FCSR);
> > +        masm.as_ext(ScratchRegister, ScratchRegister, 16, 1);
> > +        masm.ma_or(output, ScratchRegister);
> > +        masm.ma_b(output, Imm32(0), ool->entry(), Assembler::NotEqual);
> 
> If we try to do an unsigned conversion of INT32_MAX + 1, this will jump to
> the OOL entry because it's outside the range of truncld/truncls and
> InvalidOperation will be set in FCSR, right? If so, I think it will directly
> trap there, but it shouldn't. Am I missing something?
> 
> Reading the MIPS architecture manual, it seems that truncld/truncls accepts
> ranges from INT64_MIN to INT64_MAX on both platforms, which contradicts your
> comment above. If so, I think the code of this entire function can be in
> mips-shared.
The truncls/truncld used for unsigned conversion, If NaN, the invalid bit be set, and check the hi bits of trunc result for overflow. I will fix comment here.

> 
> Also, what about values in [INT64_MAX; INT64_MAX + 1[, which will trigger
> the invalid operation, but should actually properly truncate to INT64MAX?
> (and the equivalent for INT64_MIN)
> 
> @@ +328,5 @@
> > +        masm.as_ext(ScratchRegister, ScratchRegister, 16, 1);
> > +        masm.ma_or(output, ScratchRegister);
> > +        masm.ma_b(output, Imm32(0), ool->entry(), Assembler::NotEqual);
> > +
> > +        masm.moveFromFloat32(ScratchDoubleReg, output);
> 
> Just to be sure here: do we use moveFromFloat32 just to load the low 32 bits
> here, while on mips64 we can use moveFromDouble because we've tested the
> high bits are all 0? If so, for the same reason, I think we could use
> moveFromDouble on both platforms?
I think we could use moveFromFloat32 on both platform. (moveFromDouble not support in mips32.)
python2 ./jit_test.py -f --tbpl /tmp/js.64 wasm                              [126|  0|  0|  0] 100% ==============================================>|  18.5s
PASSED ALL

Thank you review.
Attachment #8752536 - Attachment is obsolete: true
Attachment #8754646 - Flags: review?(bbouvier)
Comment on attachment 8754646 [details] [diff] [review]
Part 3 - Implement WasmTruncateToInt32.

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

Thank you for the explanation and for the updated patch!

::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
@@ +216,5 @@
> +    FloatRegister input = ool->input();
> +    MIRType fromType = ool->fromType();
> +
> +    // When the input value is Infinity, NaN, or rounds to an integer outside the
> +    // range [INT32_MIN; INT32_MAX + 1[, the Invalid Operation flag is set in the FCSR.

Just to make sure we're on the same page: is the range ]INT32_MIN - 1; INT32_MAX + 1[? Ditto for the int64 range much below. Or is it that no float64 can represent values in the range ]INT64_MIN - 1; INT64_MIN]?

@@ +1359,5 @@
> +        masm.moveFromFloat32(ScratchDoubleReg, output);
> +        return;
> +    }
> +
> +    emitWasmSignedTruncateToInt32(ool, output);

Feel free to inline emitWasmSignedTruncateToInt32 here directly and then remove the emit method.

@@ +1393,5 @@
> +
> +                masm.loadConstantDouble(double(INT32_MIN), ScratchDoubleReg);
> +                masm.branchDouble(Assembler::DoubleGreaterThan, input, ScratchDoubleReg, &fail);
> +
> +                masm.as_truncwd(ScratchFloat32Reg, ScratchDoubleReg);

I guess that this works because the float32 and double scratch regs don't alias on MIPS?
Attachment #8754646 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> Comment on attachment 8754646 [details] [diff] [review]
> Part 3 - Implement WasmTruncateToInt32.
> 
> Review of attachment 8754646 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the explanation and for the updated patch!
> 
> ::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
> @@ +216,5 @@
> > +    FloatRegister input = ool->input();
> > +    MIRType fromType = ool->fromType();
> > +
> > +    // When the input value is Infinity, NaN, or rounds to an integer outside the
> > +    // range [INT32_MIN; INT32_MAX + 1[, the Invalid Operation flag is set in the FCSR.
> 
> Just to make sure we're on the same page: is the range ]INT32_MIN - 1;
> INT32_MAX + 1[? Ditto for the int64 range much below. Or is it that no
> float64 can represent values in the range ]INT64_MIN - 1; INT64_MIN]?
Outside the range [INT32_MIN; INT32_MAX + 1[ == LessThan INT32_MIN or GreaterThanOrEqual INT32_MAX + 1
> 
> @@ +1359,5 @@
> > +        masm.moveFromFloat32(ScratchDoubleReg, output);
> > +        return;
> > +    }
> > +
> > +    emitWasmSignedTruncateToInt32(ool, output);
> 
> Feel free to inline emitWasmSignedTruncateToInt32 here directly and then
> remove the emit method.
OK.

> 
> @@ +1393,5 @@
> > +
> > +                masm.loadConstantDouble(double(INT32_MIN), ScratchDoubleReg);
> > +                masm.branchDouble(Assembler::DoubleGreaterThan, input, ScratchDoubleReg, &fail);
> > +
> > +                masm.as_truncwd(ScratchFloat32Reg, ScratchDoubleReg);
> 
> I guess that this works because the float32 and double scratch regs don't
> alias on MIPS?
trunc.w.d $f0, $f0 works on mips.
To be complete, I think that values within ]INT32_MIN - 1; INT32_MIN] should be correctly truncated to INT32_MIN. If we don't have a test for this, we should add one. Or maybe your code handles this somehow?

Regarding the last truncwd operation: won't ScratchDoubleReg contain double(INT32_MIN), thus truncating ScratchFloat32Reg won't produce the expected result?
(In reply to Benjamin Bouvier [:bbouvier] from comment #10)
> To be complete, I think that values within ]INT32_MIN - 1; INT32_MIN] should
> be correctly truncated to INT32_MIN. If we don't have a test for this, we
> should add one. Or maybe your code handles this somehow?
Currently, ]INT32_MIN -1; INT32_MIN truncated to INT32_MIN in OOL.

> 
> Regarding the last truncwd operation: won't ScratchDoubleReg contain
> double(INT32_MIN), thus truncating ScratchFloat32Reg won't produce the
> expected result?
The last truncwd operation in OOL set default value(INT32_MIN) in ScratchFloat32Reg, jump to inline code. and then

masm.moveFromFloat32(ScratchFloat32Reg, output);

move INT32_MIN to output register.
I see, thank you for the explanation.
https://hg.mozilla.org/mozilla-central/rev/d0ff3a542229
https://hg.mozilla.org/mozilla-central/rev/200868644538
https://hg.mozilla.org/mozilla-central/rev/0b040f54d643
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.