Work around bug in xgetbv for Visual Studio.

source/cpu_id.cc

Index: source/cpu_id.cc
diff --git a/source/cpu_id.cc b/source/cpu_id.cc
index 2fdb82ab0e3bd6ccaeb3e7461e7a86cd0c3f4329..b23824b4b68f97f8ff735b140e1c78ec2d07cafb 100644
--- a/source/cpu_id.cc
+++ b/source/cpu_id.cc
@@ -48,7 +48,7 @@ extern "C" {
     !defined(__pnacl__) && !defined(__CLR_VER)
 LIBYUV_API
 void CpuId(uint32 info_eax, uint32 info_ecx, uint32* cpu_info) {
-#if (defined(_MSC_VER) && !defined(__clang__)) && !defined(__clang__)
+#if defined(_MSC_VER) && !defined(__clang__)
 // Visual C version uses intrinsic or inline x86 assembly.
 #if (_MSC_FULL_VER >= 160040219)

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

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.

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

Acknowledged.

   __cpuidex((int*)(cpu_info), info_eax, info_ecx);
@@ -63,7 +63,7 @@ void CpuId(uint32 info_eax, uint32 info_ecx, uint32* cpu_info) {
     mov        [edi + 8], ecx
     mov        [edi + 12], edx
   }
-#else
+#else  // Visual C but not x86
   if (info_ecx == 0) {
     __cpuid((int*)(cpu_info), info_eax);
   } else {
@@ -71,7 +71,7 @@ void CpuId(uint32 info_eax, uint32 info_ecx, uint32* cpu_info) {
   }
 #endif
 // GCC version uses inline x86 assembly.
-#else  // (defined(_MSC_VER) && !defined(__clang__)) && !defined(__clang__)
+#else  // defined(_MSC_VER) && !defined(__clang__)
   uint32 info_ebx, info_edx;
   asm volatile (
 #if defined( __i386__) && defined(__PIC__)
@@ -89,7 +89,7 @@ void CpuId(uint32 info_eax, uint32 info_ecx, uint32* cpu_info) {
   cpu_info[1] = info_ebx;
   cpu_info[2] = info_ecx;
   cpu_info[3] = info_edx;
-#endif  // (defined(_MSC_VER) && !defined(__clang__)) && !defined(__clang__)
+#endif  // defined(_MSC_VER) && !defined(__clang__)
 }
 #else  // (defined(_M_IX86) || defined(_M_X64) ...
 LIBYUV_API
@@ -98,7 +98,10 @@ void CpuId(uint32 eax, uint32 ecx, uint32* cpu_info) {
 }
 #endif
 
-// TODO(fbarchard): Enable xgetbv when validator supports it.
+// For VS2010 and earlier emit can be used:
+//   _asm _emit 0x0f _asm _emit 0x01 _asm _emit 0xd0  // For VS2010 and earlier.
+// For VS2013 32 bit, the _xgetbv(0) optimizer bug affecting return can use:
+// pragma optimize("g", off)

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

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]

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]

Done.

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

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.

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

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]

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

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

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.

 #if (defined(_M_IX86) || defined(_M_X64) || \
     defined(__i386__) || defined(__x86_64__)) && \
     !defined(__pnacl__) && !defined(__CLR_VER) && !defined(__native_client__)
@@ -106,14 +109,14 @@ void CpuId(uint32 eax, uint32 ecx, uint32* cpu_info) {
 // X86 CPUs have xgetbv to detect OS saves high parts of ymm registers.
 int GetXCR0() {
   uint32 xcr0 = 0u;
-#if (defined(_MSC_VER) && !defined(__clang__)) && (_MSC_FULL_VER >= 160040219)
-  xcr0 = (uint32)(_xgetbv(0));  // VS2010 SP1 required.
-#elif defined(_M_IX86) && defined(_MSC_VER) && !defined(__clang__)
+#if defined(_M_IX86)
   __asm {
     xor        ecx, ecx    // xcr 0
-    _asm _emit 0x0f _asm _emit 0x01 _asm _emit 0xd0  // For VS2010 and earlier.
+    xgetbv
     mov        xcr0, eax
   }
+#elif (_MSC_FULL_VER >= 160040219)
+  xcr0 = (uint32)(_xgetbv(0));  // VS2010 SP1 required.
 #elif defined(__i386__) || defined(__x86_64__)
   asm(".byte 0x0f, 0x01, 0xd0" : "=a" (xcr0) : "c" (0) : "%edx");
 #endif  // defined(__i386__) || defined(__x86_64__)