Bug 97008 - [BlackBerry] Allow denormal floats in ARM VFP
Summary: [BlackBerry] Allow denormal floats in ARM VFP
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P3 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-18 06:19 PDT by Cosmin Truta
Modified: 2012-09-27 00:00 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.90 KB, patch)
2012-09-23 19:29 PDT, Cosmin Truta
fpizlo: review-
fpizlo: commit-queue-
Details | Formatted Diff | Diff
Patch (1.97 KB, patch)
2012-09-26 21:40 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cosmin Truta 2012-09-18 06:19:10 PDT
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.
Comment 1 Cosmin Truta 2012-09-23 19:29:40 PDT
Created attachment 165301 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-23 19:32:45 PDT
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.
Comment 3 Filip Pizlo 2012-09-23 20:23:37 PDT
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.
Comment 4 Gavin Barraclough 2012-09-24 02:22:55 PDT
+1 to Filip's comment.
Comment 5 Yong Li 2012-09-24 07:25:03 PDT
(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?
Comment 6 Filip Pizlo 2012-09-24 15:22:29 PDT
(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.
Comment 7 Cosmin Truta 2012-09-24 19:04:47 PDT
(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.
Comment 8 Cosmin Truta 2012-09-26 21:40:11 PDT
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.
Comment 9 Filip Pizlo 2012-09-26 23:50:42 PDT
Comment on attachment 165925 [details]
Patch

I like this approach much better!
Comment 10 Filip Pizlo 2012-09-26 23:51:06 PDT
(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.
Comment 11 WebKit Review Bot 2012-09-27 00:00:52 PDT
Comment on attachment 165925 [details]
Patch

Clearing flags on attachment: 165925

Committed r129730: <http://trac.webkit.org/changeset/129730>
Comment 12 WebKit Review Bot 2012-09-27 00:00:57 PDT
All reviewed patches have been landed.  Closing bug.