Closed Bug 1112616 Opened 7 years ago Closed 7 years ago

Fix FPU settings in compilation threads.

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: nbp, Assigned: andy.zsshen)

References

Details

Attachments

(1 file, 2 obsolete files)

Apparently Bug 1104658 is blocked because the compilation thread does not have the same precision as the main thread, which cause Math.asin(Math.tanh(1)) to have a different output  when it is pre-computed by the compiler.

Hopefully, Shu solved a similar issue in Bug 1022948.
Attached patch HelperThread_FixFPU.txt (obsolete) — Splinter Review
Try to eliminate the precision gap between main thread and Ion compilcation thread.
Attachment #8539673 - Flags: review?(jwalden+bmo)
Comment on attachment 8539673 [details] [diff] [review]
HelperThread_FixFPU.txt

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

::: js/src/vm/HelperThreads.cpp
@@ +19,5 @@
>  #include "jscntxtinlines.h"
>  #include "jscompartmentinlines.h"
>  #include "jsobjinlines.h"
>  #include "jsscriptinlines.h"
> +#include "jsnum.h" // For FIX_FPU()

This is most certainly in the wrong spot and will trigger style violations: try |make check-style|.  You want this alphabetical up higher with other "js*.*" includes.

@@ +971,5 @@
>      }
>  #endif
>  
> +    // Set the FPU control word to be the same as the main thread's, or there
> +    // will be precision problem when the helper thread handles Ion compilation.

Probably worth adding "See bug 1104658." or something citing a bug that this fixes.
Attachment #8539673 - Flags: review?(jwalden+bmo) → review+
Attached patch HelperThread_FixFPU.txt (obsolete) — Splinter Review
1. Adjust the alphabetical order for "js.*" include statments.
2. Cite the bug 1104658 for clear comment.
Attachment #8539673 - Attachment is obsolete: true
Attachment #8539767 - Flags: review?(jwalden+bmo)
Comment on attachment 8539767 [details] [diff] [review]
HelperThread_FixFPU.txt

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

Just make the comment-fix and post that patch with checkin-needed -- I don't need to see this again.  :-)

::: js/src/vm/HelperThreads.cpp
@@ +972,5 @@
>  #endif
>  
> +    // See bug 1104658.
> +    // Set the FPU control word to be the same as the main thread's, or there
> +    // will be precision problem when the helper thread handles Ion compilation.

...or math computations on this thread may use incorrect precision rules during Ion compilation.
Attachment #8539767 - Flags: review?(jwalden+bmo) → review+
You should also make sure the patch has a correctly formatted author name and commit message; see also https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
This patch passed the try server testing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e30f9c32eb3d

Request for the help to check it in.

Thanks!
Attachment #8539767 - Attachment is obsolete: true
Attachment #8544552 - Flags: checkin?
Comment on attachment 8544552 [details] [diff] [review]
Bug_1112616_Fix.patch

Carry over r+ from Waldo.

https://hg.mozilla.org/integration/mozilla-inbound/rev/38f7539e3172

Nice work ;)
Attachment #8544552 - Flags: review+
Attachment #8544552 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/38f7539e3172
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.