Bug 70517 – Implement SSE denormal disabler for windows.

• Component: New Bugs
• Status: RESOLVED FIXED

[Raymond Toy]

Implement SSE denormal disabler for windows.

[Raymond Toy]

Created attachment 111796 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 111797 [details]
Patch

[Raymond Toy]

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 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]

Created attachment 111838 [details]
Patch

[Raymond Toy]

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 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 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]

(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 on attachment 111838 [details]
Patch

Clearing flags on attachment: 111838

Committed r98029: <http://trac.webkit.org/changeset/98029>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.