Bug 97008 – [BlackBerry] Allow denormal floats in ARM VFP

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Cosmin Truta]

By default, the ARM FPU runs in FastZero mode, which is not IEEE-compliant. In this mode, mathematical routines that handle denormal floats do not work correctly.
By clearing the FastZero flag, the IEEE mode will be enabled.

[Cosmin Truta]

Created attachment 165301 [details]
Patch

[WebKit Review Bot]

Attachment 165301 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/runtime/JSGlobalData.cpp:133:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/JavaScriptCore/runtime/JSGlobalData.cpp:134:  __volatile__ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/runtime/JSGlobalData.cpp:136:  __volatile__ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 165301 [details]
Patch

I don't like the way you've done this.

1) If you need to do things to your CPU to make it conform, then wouldn't it be better to do those things in the code that uses JavaScriptCore rather than in JavaScriptCore itself?  The way you've implemented it, a program that uses JavaScriptCore will start out having one kind of FP semantics, until the first JSGlobalData is instantiated, at which point it will henceforth have a different kind of FP semantics.  That's weird.  Wouldn't it be better to have the client program set up the FP state that it wants at the beginning (equivalent of main())?

2) If you really must put this into JavaScriptCore, say because you don't want the client program to care about what kinds of FP settings JavaScriptCore wants, then wouldn't it be infinitely better to put it someplace like initializeThreading()?  Changing the FP state should have nothing to do with creating a new JSC instance, especially since this is by definition a one-way state change - JSGlobalData::~JSGlobalData does not reset the FP state, and you don't flip it in any way on entry into this particular JSGlobalData instance.

Please consider doing either (1) or (2) instead of this current approach.

[Gavin Barraclough]

+1 to Filip's comment.

[Yong Li]

(In reply to comment #3)
> (From update of attachment 165301 [details])
> I don't like the way you've done this.
> 
> 1) If you need to do things to your CPU to make it conform, then wouldn't it be better to do those things in the code that uses JavaScriptCore rather than in JavaScriptCore itself?  The way you've implemented it, a program that uses JavaScriptCore will start out having one kind of FP semantics, until the first JSGlobalData is instantiated, at which point it will henceforth have a different kind of FP semantics.  That's weird.  Wouldn't it be better to have the client program set up the FP state that it wants at the beginning (equivalent of main())?
> 
> 2) If you really must put this into JavaScriptCore, say because you don't want the client program to care about what kinds of FP settings JavaScriptCore wants, then wouldn't it be infinitely better to put it someplace like initializeThreading()?  Changing the FP state should have nothing to do with creating a new JSC instance, especially since this is by definition a one-way state change - JSGlobalData::~JSGlobalData does not reset the FP state, and you don't flip it in any way on entry into this particular JSGlobalData instance.
> 
> Please consider doing either (1) or (2) instead of this current approach.

We thought about using initializeThreading() or doing it in the caller.  However it doesn't cover Web Worker threads. So either we need to add the same code to wtf/ThreadingPosix.cpp, or to WebCore web worker code. Which way do you think is better?

[Filip Pizlo]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 165301 [details] [details])
> > I don't like the way you've done this.
> > 
> > 1) If you need to do things to your CPU to make it conform, then wouldn't it be better to do those things in the code that uses JavaScriptCore rather than in JavaScriptCore itself?  The way you've implemented it, a program that uses JavaScriptCore will start out having one kind of FP semantics, until the first JSGlobalData is instantiated, at which point it will henceforth have a different kind of FP semantics.  That's weird.  Wouldn't it be better to have the client program set up the FP state that it wants at the beginning (equivalent of main())?
> > 
> > 2) If you really must put this into JavaScriptCore, say because you don't want the client program to care about what kinds of FP settings JavaScriptCore wants, then wouldn't it be infinitely better to put it someplace like initializeThreading()?  Changing the FP state should have nothing to do with creating a new JSC instance, especially since this is by definition a one-way state change - JSGlobalData::~JSGlobalData does not reset the FP state, and you don't flip it in any way on entry into this particular JSGlobalData instance.
> > 
> > Please consider doing either (1) or (2) instead of this current approach.
> 
> We thought about using initializeThreading() or doing it in the caller.  However it doesn't cover Web Worker threads. So either we need to add the same code to wtf/ThreadingPosix.cpp, or to WebCore web worker code. Which way do you think is better?

Why not do (1)?

Or is it the case that your OS doesn't have the notion of setting the FP mode for the entire process?

If it's the case that your OS doesn't allow you to set this for the entire process, then it seems like wtf/ThreadingPosix.cpp is the way to go.

[Cosmin Truta]

(In reply to comment #6)
> (In reply to comment #5)
> > We thought about using initializeThreading() or doing it in the caller.  However it doesn't cover Web Worker threads. So either we need to add the same code to wtf/ThreadingPosix.cpp, or to WebCore web worker code. Which way do you think is better?
> 
> Why not do (1)?
> 
> Or is it the case that your OS doesn't have the notion of setting the FP mode for the entire process?
> 
> If it's the case that your OS doesn't allow you to set this for the entire process, then it seems like wtf/ThreadingPosix.cpp is the way to go.

We need to set the FP mode per thread, and we will do this initialization in wtf/ThreadingPthreads.cpp. Thank you very much.

[Cosmin Truta]

Created attachment 165925 [details]
Patch

(In reply to comment #6)
> Or is it the case that your OS doesn't have the notion of setting the FP mode for the entire process?

We spoke with the QNX developers, who confirmed that, on ARM, every new thread starts with a default FP mode in which the FZ flag is on by default. We must clear FZ per-thread, we can't do it per-process.

[Filip Pizlo]

Comment on attachment 165925 [details]
Patch

I like this approach much better!

[Filip Pizlo]

(In reply to comment #8)
> Created an attachment (id=165925) [details]
> Patch
> 
> (In reply to comment #6)
> > Or is it the case that your OS doesn't have the notion of setting the FP mode for the entire process?
> 
> We spoke with the QNX developers, who confirmed that, on ARM, every new thread starts with a default FP mode in which the FZ flag is on by default. We must clear FZ per-thread, we can't do it per-process.

Got it.  R=me.

[WebKit Review Bot]

Comment on attachment 165925 [details]
Patch

Clearing flags on attachment: 165925

Committed r129730: <http://trac.webkit.org/changeset/129730>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.