WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
70517
Implement SSE denormal disabler for windows.
https://bugs.webkit.org/show_bug.cgi?id=70517
Summary
Implement SSE denormal disabler for windows.
Raymond Toy
Reported
2011-10-20 09:44:24 PDT
Implement SSE denormal disabler for windows.
Attachments
Patch
(5.21 KB, patch)
2011-10-20 09:49 PDT
,
Raymond Toy
no flags
Details
Formatted Diff
Diff
Patch
(5.22 KB, patch)
2011-10-20 10:02 PDT
,
Raymond Toy
no flags
Details
Formatted Diff
Diff
Patch
(4.04 KB, patch)
2011-10-20 13:24 PDT
,
Raymond Toy
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Raymond Toy
Comment 1
2011-10-20 09:49:39 PDT
Created
attachment 111796
[details]
Patch
WebKit Review Bot
Comment 2
2011-10-20 09:53:11 PDT
Attachment 111796
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:4: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Raymond Toy
Comment 3
2011-10-20 10:02:25 PDT
Created
attachment 111797
[details]
Patch
Raymond Toy
Comment 4
2011-10-20 10:05:00 PDT
This implements the denormal disabler for Windows similar to what is done on Mac and Linux. Chris, although we talked about not checking, I decided that we should check if SSE2 is available before we try to run instructions that might not be available. I didn't want to cause crashes for people running chrome on windows on old machines without SSE2. The same check is done for Mac/Linux because I merged most of the code into one implementation.
Kenneth Russell
Comment 5
2011-10-20 10:28:15 PDT
Comment on
attachment 111797
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=111797&action=review
> Source/WebCore/platform/audio/DenormalDisabler.h:39 > #if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))
Are you sure that inline assembly is supported in the 64-bit MSVC compiler?
http://stackoverflow.com/questions/2980674/fesetround-with-msvc-x64
indicates it isn't. Could you use Microsoft-specific intrinsics instead of assembly? See
http://msdn.microsoft.com/en-gb/library/e9b52ceh.aspx
.
Raymond Toy
Comment 6
2011-10-20 13:24:35 PDT
Created
attachment 111838
[details]
Patch
Raymond Toy
Comment 7
2011-10-20 13:27:03 PDT
Comment on
attachment 111797
[details]
Patch _controlfp_s intrinsic is used now. No inline assembly is used because it is not available on x64. (
http://msdn.microsoft.com/en-us/library/wbk4z78b.aspx
). Ripped out the check for SSE2 since we're not doing that on Windows and weren't doing it on Linux/Mac previously.
Kenneth Russell
Comment 8
2011-10-20 13:40:26 PDT
Comment on
attachment 111838
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=111838&action=review
Looks good to me. One question, but r=me.
> Source/WebCore/platform/audio/DenormalDisabler.h:55 > + _controlfp_s(&unused, _DN_FLUSH, _MCW_DN);
Since the guard in flushDenormalFloatToZero includes (!_M_IX86_FP), would it make sense to elide this block and the _controlfp_s call in the destructor if (!_M_IX86_FP)?
Raymond Toy
Comment 9
2011-10-20 13:49:31 PDT
Comment on
attachment 111838
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=111838&action=review
>> Source/WebCore/platform/audio/DenormalDisabler.h:55 >> + _controlfp_s(&unused, _DN_FLUSH, _MCW_DN); > > Since the guard in flushDenormalFloatToZero includes (!_M_IX86_FP), would it make sense to elide this block and the _controlfp_s call in the destructor if (!_M_IX86_FP)?
I thought about that, but the OS(WINDOWS) and COMPILER(MSVC) makes it clear this is a windows thing, which is harder to tell if you just had !_M_IX86_FP. But at some expense in readability and duplication. No problem if you think changing it makes it better.
Kenneth Russell
Comment 10
2011-10-20 13:58:45 PDT
(In reply to
comment #9
)
> (From update of
attachment 111838
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=111838&action=review
> > >> Source/WebCore/platform/audio/DenormalDisabler.h:55 > >> + _controlfp_s(&unused, _DN_FLUSH, _MCW_DN); > > > > Since the guard in flushDenormalFloatToZero includes (!_M_IX86_FP), would it make sense to elide this block and the _controlfp_s call in the destructor if (!_M_IX86_FP)? > > I thought about that, but the OS(WINDOWS) and COMPILER(MSVC) makes it clear this is a windows thing, which is harder to tell if you just had !_M_IX86_FP. But at some expense in readability and duplication.
>
> No problem if you think changing it makes it better.
I see. What I was suggesting was to add the #if (!_M_IX86_FP) include guard inside the existing OS(WINDOWS) include guards. It's fine as is; the extra work on non-SSE2 platforms should be negligible.
WebKit Review Bot
Comment 11
2011-10-20 15:38:07 PDT
Comment on
attachment 111838
[details]
Patch Clearing flags on attachment: 111838 Committed
r98029
: <
http://trac.webkit.org/changeset/98029
>
WebKit Review Bot
Comment 12
2011-10-20 15:38:11 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug