Work around bug in xgetbv for Visual Studio.

Work around bug in xgetbv for Visual Studio.

xgetbv is generating bad code, falsely disabling AVX2 and AVX512.
disable optimization for the function affected on older versions of Visual C 32 bit.

R=brucedawson@chromium.org, dhrosa@google.com, harryjin@google.com
BUG=libyuv:529
Committed: https://chromium.googlesource.com/libyuv/libyuv/+/fae1a105458a23d42c3e30f4d9ca321d08934e80

[Diony Rosa, 2015-12-08 15:32:48]

dhrosa@google.com changed reviewers:
+ dhrosa@google.com

[Diony Rosa, 2015-12-08 15:32:49]

lgtm

[fbarchard, 2015-12-08 18:48:25]

fbarchard@chromium.org changed reviewers:
+ fbarchard@chromium.org

[fbarchard, 2015-12-08 18:48:25]

Tested with emulator on various CPUs

skylake xeon
c:\intelsde\sde -mix -omix mix.txt -ast -oast ast.txt -skx --
out\release\libyuv_unittest.exe --gtest_catch_exceptions=0
--gtest_filter=*CpuHas

[ RUN      ] LibYUVBaseTest.TestCpuHas
xgetbv xcr0 255
xgetbv xcr0 255
Cpu Flags 3ff1
Has ARM 0
Has NEON 0
Has X86 10
Has SSE2 20
Has SSSE3 40
Has SSE4.1 80
Has SSE4.2 100
Has AVX 200
Has AVX2 400
Has ERMS 800
Has FMA3 1000
Has AVX3 2000
Has MIPS 0
Has MIPS DSPR2 0
[       OK ] LibYUVBaseTest.TestCpuHas (119 ms)

skylake
c:\intelsde\sde -mix -omix mix.txt -ast -oast ast.txt -skl --
out\release\libyuv_unittest.exe --gtest_catch_exceptions=0
--gtest_filter=*CpuHas
[ RUN      ] LibYUVBaseTest.TestCpuHas
xgetbv xcr0 31
xgetbv xcr0 31
Cpu Flags 1ff1
Has ARM 0
Has NEON 0
Has X86 10
Has SSE2 20
Has SSSE3 40
Has SSE4.1 80
Has SSE4.2 100
Has AVX 200
Has AVX2 400
Has ERMS 800
Has FMA3 1000
Has AVX3 0
Has MIPS 0
Has MIPS DSPR2 0
[       OK ] LibYUVBaseTest.TestCpuHas (102 ms)

Broadwell
c:\intelsde\sde -mix -omix mix.txt -ast -oast ast.txt -bdw --
out\release\libyuv_unittest.exe --gtest_catch_exceptions=0
--gtest_filter=*CpuHas
[ RUN      ] LibYUVBaseTest.TestCpuHas
xgetbv xcr0 7
xgetbv xcr0 7
Cpu Flags 1ff1
Has ARM 0
Has NEON 0
Has X86 10
Has SSE2 20
Has SSSE3 40
Has SSE4.1 80
Has SSE4.2 100
Has AVX 200
Has AVX2 400
Has ERMS 800
Has FMA3 1000
Has AVX3 0
Has MIPS 0
Has MIPS DSPR2 0
[       OK ] LibYUVBaseTest.TestCpuHas (102 ms)

SandyBridge
c:\intelsde\sde -mix -omix mix.txt -ast -oast ast.txt -snb --
out\release\libyuv_unittest.exe --gtest_catch_exceptions=0
--gtest_filter=*CpuHas
[ RUN      ] LibYUVBaseTest.TestCpuHas
xgetbv xcr0 7
xgetbv xcr0 7
Cpu Flags 3f1
Has ARM 0
Has NEON 0
Has X86 10
Has SSE2 20
Has SSSE3 40
Has SSE4.1 80
Has SSE4.2 100
Has AVX 200
Has AVX2 0
Has ERMS 0
Has FMA3 0
Has AVX3 0
Has MIPS 0
Has MIPS DSPR2 0
[       OK ] LibYUVBaseTest.TestCpuHas (105 ms)

Westmere
c:\intelsde\sde -mix -omix mix.txt -ast -oast ast.txt -wsm --
out\release\libyuv_unittest.exe --gtest_catch_exceptions=0
--gtest_filter=*CpuHas

[ RUN      ] LibYUVBaseTest.TestCpuHas
Cpu Flags 1f1
Has ARM 0
Has NEON 0
Has X86 10
Has SSE2 20
Has SSSE3 40
Has SSE4.1 80
Has SSE4.2 100
Has AVX 0
Has AVX2 0
Has ERMS 0
Has FMA3 0
Has AVX3 0
Has MIPS 0
Has MIPS DSPR2 0
[       OK ] LibYUVBaseTest.TestCpuHas (115 ms)

[brucedawson, 2015-12-08 19:18:46]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2015-12-08 19:18:46]

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc#newcode53
source/cpu_id.cc:53: #if (_MSC_FULL_VER >= 160040219)
Off topic, but it would be nice to stop having three versions of this code. In
particular it appears that they have slightly different behavior? If the
compiler support is there then we should be able to converge on a single
version, with no assembly language.

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc#newcod...
source/cpu_id.cc:104: // pragma optimize("g", off)
I would recommend using #pragma optimize off/on to work around this bug rather
than using inline assembly language.

My tests show that the bug is fixed in VS 2015 Update 1 (190023506) so you can
put a version check for that around the optimize disabling.

You should also open a bug on
https://connect.microsoft.com/VisualStudio/feedback/LoadSubmitFeedbackForm to
track this, include the repro, and make sure Microsoft  knows. I could not find
an existing bug for this.

[fbarchard, 2015-12-08 20:39:23]

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc#newcode53
source/cpu_id.cc:53: #if (_MSC_FULL_VER >= 160040219)
On 2015/12/08 19:18:46, brucedawson wrote:
> Off topic, but it would be nice to stop having three versions of this code. In
> particular it appears that they have slightly different behavior? If the
> compiler support is there then we should be able to converge on a single
> version, with no assembly language.

Acknowledged.

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc#newcod...
source/cpu_id.cc:104: // pragma optimize("g", off)
On 2015/12/08 19:18:46, brucedawson wrote:
> I would recommend using #pragma optimize off/on to work around this bug rather
> than using inline assembly language.
> 
> My tests show that the bug is fixed in VS 2015 Update 1 (190023506) so you can
> put a version check for that around the optimize disabling.
> 
> You should also open a bug on
> https://connect.microsoft.com/VisualStudio/feedback/LoadSubmitFeedbackForm to
> track this, include the repro, and make sure Microsoft  knows. I could not
find
> an existing bug for this.

Acknowledged.

My concern with using xgetbv with optimization off, is that the nature of the
bug is the intrinsic is misdeclared as to its register usage and return value. 
The optimizer exposes that, but its not clear if the unoptimized code has
unknown side effects.
The assembly works for all compilers except Visual Studio in 64 bit  mode.  My
tests on 64 bit didnt produce any errors.

[fbarchard, 2015-12-08 23:06:51]

disable optimization for old versions of visual c

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc#newcod...
source/cpu_id.cc:104: // pragma optimize("g", off)
On 2015/12/08 19:18:46, brucedawson wrote:
> I would recommend using #pragma optimize off/on to work around this bug rather
> than using inline assembly language.
> 
> My tests show that the bug is fixed in VS 2015 Update 1 (190023506) so you can
> put a version check for that around the optimize disabling.
> 
> You should also open a bug on
> https://connect.microsoft.com/VisualStudio/feedback/LoadSubmitFeedbackForm to
> track this, include the repro, and make sure Microsoft  knows. I could not
find
> an existing bug for this.

Done.

[brucedawson, 2015-12-09 00:10:18]

Needs a version check for VS 2015 Update 1 I think, so that the workaround is
removed for that version.

You can do test builds with VS 2015 just by setting GYP_MSVS_VERSION=2015 and
then running "gclient runhooks"

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc#newcod...
source/cpu_id.cc:104: // pragma optimize("g", off)
Unoptimized code should be quite stable so if the generated code is correct now
then that should remain true. That is, if you can tell through inspection that
the unoptimized VS 2013 code is correct then unless we change this function or
change compilers that should continue to be true. Three versions of the code
seems excessive and can easily lead to human errors.

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc#newcod...
source/cpu_id.cc:104: // pragma optimize("g", off)
Can you add a link to the bug in a comment in the code? I think that's
appropriate - otherwise compiler-bug workarounds become difficult to understand
and deal with in the future.

[fbarchard, 2015-12-09 01:08:57]

gclangcl build ignores pragma and generates

_GetXCR0:
  00000000: 31 C9              xor         ecx,ecx
  00000002: 0F 01 D0           xgetbv
  00000005: C3                 ret

Visual C with intrinsic and pragma optimize off
 00000000: 55                 push        ebp
 00000001: 8B EC              mov         ebp,esp
 00000003: 51                 push        ecx
 00000004: C7 45 FC 00 00 00  mov         dword ptr [ebp-4],0
           00
 0000000B: 33 C9              xor         ecx,ecx
 0000000D: 0F 01 D0           xgetbv
 00000010: 89 45 FC           mov         dword ptr [ebp-4],eax
 00000013: 8B 45 FC           mov         eax,dword ptr [ebp-4]
 00000016: 8B E5              mov         esp,ebp
 00000018: 5D                 pop         ebp
 00000019: C3                 ret

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc#newcod...
source/cpu_id.cc:104: // pragma optimize("g", off)
On 2015/12/09 00:10:18, brucedawson wrote:
> Can you add a link to the bug in a comment in the code? I think that's
> appropriate - otherwise compiler-bug workarounds become difficult to
understand
> and deal with in the future.

Done.  (already had link, but youre looking at old version here)

https://codereview.chromium.org/1503393004/diff/40001/source/cpu_id.cc#newcod...
source/cpu_id.cc:104: // pragma optimize("g", off)
On 2015/12/09 00:10:18, brucedawson wrote:
> Unoptimized code should be quite stable so if the generated code is correct
now
> then that should remain true. That is, if you can tell through inspection that
> the unoptimized VS 2013 code is correct then unless we change this function or
> change compilers that should continue to be true. Three versions of the code
> seems excessive and can easily lead to human errors.

Done.
The 3 versions correspond to 3 compilers that are incompatible.
old visual c that doesnt support intrinsics.
new visual c that doesnt support inline
gcc that doesnt support intrinsic and has different inline syntax.
i've dropped support for the old visual c (x32 inline), where support for avx2
doesnt work anyway.

[fbarchard, 2015-12-09 01:26:36]

Description was changed from

==========
Work around bug in xgetbv for Visual Studio.

xgetbv is generating bad code, falsely disabling AVX2 and AVX512.
Use asm version of xgetbv for 32 bit Visual C to avoid an optimizer bug
affecting the intrinsic xgetbv(0)

R=harryjin@google.com
BUG=libyuv:529
==========

to

==========
Work around bug in xgetbv for Visual Studio.

xgetbv is generating bad code, falsely disabling AVX2 and AVX512.
disable optimization for the function affected on older versions of Visual C 32
bit.

R=harryjin@google.com
BUG=libyuv:529
==========

[brucedawson, 2015-12-09 01:53:34]

lgtm

https://codereview.chromium.org/1503393004/diff/60001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/1503393004/diff/60001/source/cpu_id.cc#newcod...
source/cpu_id.cc:129: // Return optimization to previous setting, which is off
for debug.
I'd delete the ", which is off for debug." from the comment.

[fbarchard, 2015-12-09 02:13:21]

Caveat this version no longer works on vs2010 or earlier.
vs2010 sp1 required.

Older versions of clang also dont work, but current versions in chromium do.

Will need to watch for build errors.

The previous code had potential for bug on new cpus (Haswell+) on older OS that
does not support the new registers.  If 2 threads tried to use AVX2, they would
corrupt each others register stat.

https://codereview.chromium.org/1503393004/diff/60001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/1503393004/diff/60001/source/cpu_id.cc#newcod...
source/cpu_id.cc:129: // Return optimization to previous setting, which is off
for debug.
On 2015/12/09 01:53:34, brucedawson wrote:
> I'd delete the ", which is off for debug." from the comment.

Done.

[fbarchard1, 2015-12-09 02:13:35]

Description was changed from

==========
Work around bug in xgetbv for Visual Studio.

xgetbv is generating bad code, falsely disabling AVX2 and AVX512.
disable optimization for the function affected on older versions of Visual C 32
bit.

R=harryjin@google.com
BUG=libyuv:529
==========

to

==========
Work around bug in xgetbv for Visual Studio.

xgetbv is generating bad code, falsely disabling AVX2 and AVX512.
disable optimization for the function affected on older versions of Visual C 32
bit.

R=brucedawson@chromium.org, dhrosa@google.com, harryjin@google.com
BUG=libyuv:529

Committed:
https://chromium.googlesource.com/libyuv/libyuv/+/fae1a105458a23d42c3e30f4d9c...
==========

[fbarchard1, 2015-12-09 02:13:36]

Committed patchset #5 (id:80001) manually as
fae1a105458a23d42c3e30f4d9ca321d08934e80 (presubmit successful).