Closed Bug 1465770 Opened Last year Closed Last year

mips64 convertDoubleToInt32 cp1_fcsr error

Categories

(Core :: JavaScript Engine, defect)

61 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: qiaopengcheng-hf, Assigned: qiaopengcheng-hf)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Fedora; Linux mips64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20180227202148

Steps to reproduce:

cd jit-test
./jit-test --tbpl ../release_OPT.OBJ/dist/bin/js bug1060387.js


Actual results:

/home/qiao/work_qiao/open_src/inbound/js/src/jit-test/tests/ion/bug1060387.js:18:5 Error: Assertion failed: got "{\"0\":\"0\",\"1073741825\":\"1073741825\",\"2147483647\":\"2147483648\"}", expected "{\"0\":\"0\",\"1073741825\":\"1073741825\",\"2147483648\":\"2147483648\"}"
Stack:
  foo@/home/qiao/work_qiao/open_src/inbound/js/src/jit-test/tests/ion/bug1060387.js:18:5
  @/home/qiao/work_qiao/open_src/inbound/js/src/jit-test/tests/ion/bug1060387.js:20:1


Expected results:

should PASS all.
When mips  instruction trunc.w.d   trigging an invalid operation, 
the Invalid Operation flag is set in the FCSR.   
The flag is V-flag-bit in the cp1-FCSR  bit16, not the I-flag-bit.
Attachment #8982208 - Flags: review?(jdemooij)
Comment on attachment 8982208 [details] [diff] [review]
mips64 convertDoubleToInt32 cp1_fcsr error

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

Forwarding to a MIPS expert.
Attachment #8982208 - Flags: review?(jdemooij) → review?(dragan.mladjenovic)
Version: 62 Branch → 61 Branch
Comment on attachment 8982208 [details] [diff] [review]
mips64 convertDoubleToInt32 cp1_fcsr error

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

::: js/src/jit/mips64/MacroAssembler-mips64.cpp
@@ +116,4 @@
>      as_truncwd(ScratchFloat32Reg, src);
>      as_cfc1(ScratchRegister, Assembler::FCSR);
>      moveFromFloat32(ScratchFloat32Reg, dest);
> +    ma_ext(ScratchRegister, ScratchRegister, Assembler::CauseV, 1);

Thanks for the patch. Sorry for introducing this bug. I had an buggy FPU emulator that would set both V and I flag when dealing with out of range values. I believe that correct solution for this issue seems to be changing the length of ext to 5 in order to cover the both I and V flags. Basically we should take the failure path when either I or V cause flag is present. This should be also done for Float32 and for Mips32 implementation also.

I took the liberty to retag this as issue for version 61 because my buggy change first landed on that branch.
Attachment #8982208 - Flags: feedback?(qiaopengcheng-hf)
(In reply to Dragan Mladjenovic from comment #3)
> Comment on attachment 8982208 [details] [diff] [review]
> mips64 convertDoubleToInt32 cp1_fcsr error
> 
> Review of attachment 8982208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips64/MacroAssembler-mips64.cpp
> @@ +116,4 @@
> >      as_truncwd(ScratchFloat32Reg, src);
> >      as_cfc1(ScratchRegister, Assembler::FCSR);
> >      moveFromFloat32(ScratchFloat32Reg, dest);
> > +    ma_ext(ScratchRegister, ScratchRegister, Assembler::CauseV, 1);
> 
> Thanks for the patch. Sorry for introducing this bug. I had an buggy FPU
> emulator that would set both V and I flag when dealing with out of range
> values. I believe that correct solution for this issue seems to be changing
> the length of ext to 5 in order to cover the both I and V flags. Basically
> we should take the failure path when either I or V cause flag is present.
> This should be also done for Float32 and for Mips32 implementation also.
> 
> I took the liberty to retag this as issue for version 61 because my buggy
> change first landed on that branch.

Thanks!

For the mips-arch offical-release user-manual, I think mips-arch processor maybe not work like this at least here for int32.
The following is the mips offical-release user-manual descripting,

TRUNC.W.D fd, fs

Purpose: Floating Point Truncate to Word Fixed Point
To convert an FP value to 32-bit fixed point, rounding toward zero

Description: FPR[fd] ← convert_and_round(FPR[fs])
The value in FPRfs, in format fmt, is converted to a value in 32-bit word fixed point format using rounding toward
zero (rounding mode 1). The result is placed in FPR fd .
When the source value is Infinity, NaN, or rounds to an integer outside the range -2^31 to 2^31-1, the result cannot be
represented correctly and an IEEE Invalid Operation condition exists. In this case the Invalid Operation flag is set in
the FCSR. If the Invalid Operation Enable bit is set in theFCSR, no result is written to fd and an Invalid Operation
exception is taken immediately. Otherwise, the default result, 2^31–1, is written to fd .


From: MD00087-2B-MIPS64BIS-AFP-05.04.pdf


Of course, the I-flag should be considerd.
Tomorrow morning I'll have more testing.
Attachment #8982208 - Flags: feedback+
Attachment #8982208 - Flags: feedback?(qiaopengcheng-hf) → feedback+
Attachment #8982208 - Attachment is obsolete: true
Attachment #8982208 - Flags: review?(dragan.mladjenovic)
Attachment #8982395 - Flags: review?(dragan.mladjenovic)
Comment on attachment 8982395 [details] [diff] [review]
mips64 convertDoubleToInt32 cp1_fcsr error

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

::: js/src/jit/mips64/MacroAssembler-mips64.cpp
@@ +116,5 @@
>      as_truncwd(ScratchFloat32Reg, src);
>      as_cfc1(ScratchRegister, Assembler::FCSR);
>      moveFromFloat32(ScratchFloat32Reg, dest);
> +    ma_ext(ScratchRegister, ScratchRegister, Assembler::CauseI, 6);
> +    as_andi(ScratchRegister, ScratchRegister, 0x11);//masking for Inexact and Invalid flag.

Sorry for the late response. The patch looks good. The same fix should be applied for MacroAssemblerMIPS64Compat::convertFloat32ToInt32, MacroAssemblerMIPSCompat::convertFloat32ToInt32 and MacroAssemblerMIPSCompat::convertDoubleToInt32. The masking might be redundant given that the document  lists only Inexact, Invalid Operation, Unimplemented Operation under Floating Point Exceptions for trunc.*.* instructions, but I guess it won't hurt to leave it as it is as a precaution.
Attachment #8982395 - Flags: review?(dragan.mladjenovic) → review+
Keywords: checkin-needed
(In reply to Dragan Mladjenovic from comment #6)
> Comment on attachment 8982395 [details] [diff] [review]
> mips64 convertDoubleToInt32 cp1_fcsr error
> 
> Review of attachment 8982395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips64/MacroAssembler-mips64.cpp
> @@ +116,5 @@
> >      as_truncwd(ScratchFloat32Reg, src);
> >      as_cfc1(ScratchRegister, Assembler::FCSR);
> >      moveFromFloat32(ScratchFloat32Reg, dest);
> > +    ma_ext(ScratchRegister, ScratchRegister, Assembler::CauseI, 6);
> > +    as_andi(ScratchRegister, ScratchRegister, 0x11);//masking for Inexact and Invalid flag.
> 
> Sorry for the late response. The patch looks good. The same fix should be
> applied for MacroAssemblerMIPS64Compat::convertFloat32ToInt32,
> MacroAssemblerMIPSCompat::convertFloat32ToInt32 and
> MacroAssemblerMIPSCompat::convertDoubleToInt32. The masking might be
> redundant given that the document  lists only Inexact, Invalid Operation,
> Unimplemented Operation under Floating Point Exceptions for trunc.*.*
> instructions, but I guess it won't hurt to leave it as it is as a precaution.

Thanks~
I agree with you.

Should I uploading other patches for these function which applied for mips32 ?
> MacroAssemblerMIPSCompat::convertFloat32ToInt32 and
> MacroAssemblerMIPSCompat::convertDoubleToInt32.
(In reply to qiaopengcheng from comment #7)
> (In reply to Dragan Mladjenovic from comment #6)
> > Comment on attachment 8982395 [details] [diff] [review]
> > mips64 convertDoubleToInt32 cp1_fcsr error
> > 

> Should I uploading other patches for these function which applied for mips32
> ?
> > MacroAssemblerMIPSCompat::convertFloat32ToInt32 and
> > MacroAssemblerMIPSCompat::convertDoubleToInt32.

I would be grateful if you do so.
You can post the here and automatically mark them with review+ .
(In reply to Dragan Mladjenovic from comment #8)
> (In reply to qiaopengcheng from comment #7)
> > (In reply to Dragan Mladjenovic from comment #6)
> > > Comment on attachment 8982395 [details] [diff] [review]
> > > mips64 convertDoubleToInt32 cp1_fcsr error
> > > 
> 
> > Should I uploading other patches for these function which applied for mips32
> > ?
> > > MacroAssemblerMIPSCompat::convertFloat32ToInt32 and
> > > MacroAssemblerMIPSCompat::convertDoubleToInt32.
> 
> I would be grateful if you do so.
> You can post the here and automatically mark them with review+ .

I think the function of MacroAssemblerMIPSCompat::convertDoubleToInt32 is needed.

The function of MacroAssemblerMIPSCompat::convertFloat32ToInt32 is no need for
float32's integer range less than int32.
Attachment #8982465 - Flags: review?(dragan.mladjenovic)
Attachment #8982465 - Flags: review?(dragan.mladjenovic) → review+
(In reply to qiaopengcheng from comment #10)
> Created attachment 8982465 [details] [diff] [review]
> part2  mips32 convertDoubleToInt32 cp1_fcsr error

I believe they are needed. While float32 cannot represent full int32 range it can easily represent something like (1ll << 32) which might not trigger Inexact cause and therefore needs the same fix.
Assignee: nobody → qiaopengcheng-hf
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca30bde6447e
mips64: convertDoubleToInt32 cp1_fcsr error. r=dragan.mladjenovic
https://hg.mozilla.org/integration/mozilla-inbound/rev/af6951f3fd41
mips32: convertDoubleToInt32 cp1_fcsr error. r=dragan.mladjenovic
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca30bde6447e
https://hg.mozilla.org/mozilla-central/rev/af6951f3fd41
Status: UNCONFIRMED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(In reply to Dragan Mladjenovic from comment #11)
> (In reply to qiaopengcheng from comment #10)
> > Created attachment 8982465 [details] [diff] [review]
> > part2  mips32 convertDoubleToInt32 cp1_fcsr error
> 
> I believe they are needed. While float32 cannot represent full int32 range
> it can easily represent something like (1ll << 32) which might not trigger
> Inexact cause and therefore needs the same fix.


Yes, you are right.
(In reply to Andreea Pavel [:apavel] from comment #13)
> https://hg.mozilla.org/mozilla-central/rev/ca30bde6447e
> https://hg.mozilla.org/mozilla-central/rev/af6951f3fd41

Sorry for posting on closed issue. 
What can I do to get these changes also pushed onto 61 beta branch?
Is it to late? What is the correct workflow in this case?
Flags: needinfo?(apavel)
(In reply to qiaopengcheng from comment #14)
> (In reply to Dragan Mladjenovic from comment #11)
> > (In reply to qiaopengcheng from comment #10)
> > > Created attachment 8982465 [details] [diff] [review]
> > > part2  mips32 convertDoubleToInt32 cp1_fcsr error
> > 
> > I believe they are needed. While float32 cannot represent full int32 range
> > it can easily represent something like (1ll << 32) which might not trigger
> > Inexact cause and therefore needs the same fix.
> 
> 
> Yes, you are right.

Would you like to open a separate issue for Float32 or should I do it?
(In reply to Dragan Mladjenovic from comment #15)
> (In reply to Andreea Pavel [:apavel] from comment #13)
> > https://hg.mozilla.org/mozilla-central/rev/ca30bde6447e
> > https://hg.mozilla.org/mozilla-central/rev/af6951f3fd41
> 
> Sorry for posting on closed issue. 
> What can I do to get these changes also pushed onto 61 beta branch?
> Is it to late? What is the correct workflow in this case?

No, i don't think it's to late. I think this needs to be reviewed and approved to be pushed to beta. 

:RyanVM can you please give insight on the above statement? To push this to beta 61 we just need to add the approval-mozilla-beta flag?
Flags: needinfo?(ryanvm)
Flags: needinfo?(dragan.mladjenovic)
Flags: needinfo?(apavel)
Correct, set the approval-mozilla-beta? flag (you can do it on one patch and say it's for both) and answer the questions in the form that appear in the comments box.
Flags: needinfo?(ryanvm)
Comment on attachment 8982395 [details] [diff] [review]
mips64 convertDoubleToInt32 cp1_fcsr error

Approval Request Comment
[Feature/Bug causing the regression]: 1434843
[User impact if declined]: Affects users on mips64.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: Other patch in this issue and 1467404. 
[Is the change risky?]: No
[Why is the change risky/not risky?]: Affects only MIPS specific code.
[String changes made/needed]: None
Flags: needinfo?(dragan.mladjenovic)
Attachment #8982395 - Flags: approval-mozilla-beta?
Sounds like we don't need this on esr60, but feel free to nominate it if that's incorrect.
Comment on attachment 8982395 [details] [diff] [review]
mips64 convertDoubleToInt32 cp1_fcsr error

MIPS-only fix. Approved for 61.0b12.
Attachment #8982395 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.