Work around bug in xgetbv for Visual Studio.

source/cpu_id.cc

 /*
  *  Copyright 2011 The LibYuv Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS. All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 30 matching lines @@
 #else
 #define SAFEBUFFERS
 #endif
 
 // Low level cpuid for X86.
 #if (defined(_M_IX86) || defined(_M_X64) || \
     defined(__i386__) || defined(__x86_64__)) && \
     !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);
 #elif defined(_M_IX86)
   __asm {
     mov        eax, info_eax
     mov        ecx, info_ecx
     mov        edi, cpu_info
     cpuid
     mov        [edi], eax
     mov        [edi + 4], ebx
     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 {
     cpu_info[3] = cpu_info[2] = cpu_info[1] = cpu_info[0] = 0;
   }
 #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__)
     // Preserve ebx for fpic 32 bit.
     "mov %%ebx, %%edi                          \n"
     "cpuid                                     \n"
     "xchg %%edi, %%ebx                         \n"
     : "=D" (info_ebx),
 #else
     "cpuid                                     \n"
     : "=b" (info_ebx),
 #endif  //  defined( __i386__) && defined(__PIC__)
       "+a" (info_eax), "+c" (info_ecx), "=d" (info_edx));
   cpu_info[0] = info_eax;
   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
 void CpuId(uint32 eax, uint32 ecx, uint32* cpu_info) {
   cpu_info[0] = cpu_info[1] = cpu_info[2] = cpu_info[3] = 0;
 }
 #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__)
 #define HAS_XGETBV
 // 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__)
   return xcr0;
 }
 #endif  // defined(_M_IX86) || defined(_M_X64) ..
 
 // based on libvpx arm_cpudetect.c
 // For Arm, but public to allow testing on any CPU
 LIBYUV_API SAFEBUFFERS
@@ skipping 158 matching lines @@
 // Note that use of this function is not thread safe.
 LIBYUV_API
 void MaskCpuFlags(int enable_flags) {
   cpu_info_ = InitCpuFlags() & enable_flags;
 }
 
 #ifdef __cplusplus
 }  // extern "C"
 }  // namespace libyuv
 #endif