Adding check for OS support to AVX

Adding check for OS support to AVX

CPU is not an ideal place to put this check but it appears to be the most appropriate place given the intended usage of "has_avx()" and the lack of a "PlatformFeatures" type class.  This code checks the operating system for AVX support in addition to checking for CPU support.

BUG=171824

[DaleCurtis, 2013-03-06 01:41:00]

Assuming we want to do this, you'll want to replace at least the windows and mac
functions in this patch set with their base::win::windows_version and
base::mac::mac_util equivalents.

Also, this could be attached to the SysInfo() class, though I think base/cpu is
where people will go looking for a valid check.

[apatrick_chromium, 2013-03-06 02:02:28]

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode99
base/cpu.cc:99: if (GetVersionEx(&version_info))
Youu mean this?

if (!GetVersionEx(&version_info))

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode103
base/cpu.cc:103: return version_info.dwMajorVersion >= 6 &&
version_info.dwMinorVersion > 1;
This won't work for 7.0 and 7.1 if the major version number ever gets bumped.
Also, it doesn't check for Windows 7 SP1, just Windows 8 and Windows Server
2012, right?

Check OSInfo in base/win/windows_version.h.

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode107
base/cpu.cc:107: FILE* f = fopen("/proc/cpuinfo", "r");
I think you can use file_util::ReadFileToString here. Then there are no issues
with the fixed size buffer. Also, I think it will assert if it is called on a
thread that does not allow IO.

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode115
base/cpu.cc:115: if (strstr(buffer, "avx")) {
Does /proc/cpuinfo reflect whether the OS has AVX or just the CPU?

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode128
base/cpu.cc:128: char* kernelVersion;
nit: not chrome style

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode134
base/cpu.cc:134: kernel_version = new char[length];
scoped_ptr<char[]>

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode139
base/cpu.cc:139: delete [] kernel_version;
not needed for scoped_ptr

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode144
base/cpu.cc:144: if (sscanf(kernel_version, "%d.%d.%d", &major, &minor,
&patch_level) != 3) {
check base/version.h. It can deal with version numbers with a variable number of
components and has as IsOlderThan for comparisons.

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode146
base/cpu.cc:146: delete [] kernel_version;
not needed for scoped_ptr

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode150
base/cpu.cc:150: free(kernelVersion);
kernelVersion is uninitialized

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode153
base/cpu.cc:153: return major >= 10 && minor >= 8;
This won't work for 11.0 through 11.7.

[brettw, 2013-03-06 04:53:36]

I think this location is OK for this code. At first I thought it was wrong, but
after researching it more it seems like the best place.

Just some minor additional comments.

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode113
base/cpu.cc:113: while (fgets(buffer, (int)sizeof(buffer), f)) {
Use C++ casts.

https://codereview.chromium.org/12511002/diff/1/base/cpu.cc#newcode157
base/cpu.cc:157: return false;
indent.

[DaleCurtis, 2013-03-06 18:32:37]

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode111
base/cpu.cc:111: if (!file_util::ReadFileToString("/proc/cpuinfo", &buffer))
Is there a different way you can write this check? Doing this prevents base/cpu
from being used in the renderer side due to sandbox.

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode130
base/cpu.cc:130: if (sysctl(mib, 2, buffer, &length, NULL, 0) < 0)
Perhaps the same sandbox issue.

[apatrick_chromium, 2013-03-06 19:19:21]

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode18
base/cpu.cc:18: #if defined (OS_MACOSX)
nit: space between "defined" and "("

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode19
base/cpu.cc:19: #include "base/version.h"
#include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode23
base/cpu.cc:23: 
#if defined(OS_WIN)
#include <windows.h>
#endif

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode98
base/cpu.cc:98: OSVERSIONINFO version_info;
Consider OSVERSIONINFOEX to get service pack version. See below.

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode100
base/cpu.cc:100: if (!GetVersionEx(&version_info))
You need to set version_info.dwOSVersionInfoSize to sizeof(version_info) before
calling this.

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode103
base/cpu.cc:103: // Windows 7 SP1 added support for AVX.
This tests for Windows 7 or later rather than Windows 7 SP1 or later. There's
code in base/win/windows_version.h to get the the service pack. Alternatively,
version_info.wServicePackMajor >= 1 when Windows version is 6.1.

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode111
base/cpu.cc:111: if (!file_util::ReadFileToString("/proc/cpuinfo", &buffer))
First argument should be of type base::FilePath.

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode117
base/cpu.cc:117: #elif  defined(OS_MACOSX)
nit: two spaces between #elif and defined

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode121
base/cpu.cc:121: const static int mib[2] = {CTL_KERN, KERN_OSRELEASE};
This is passed to sysctl so it can't be const.
http://docs.oracle.com/cd/E19048-01/chorus5/806-7021/6jfu393f0/index.html

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode123
base/cpu.cc:123: if (sysctl(mib, 2, NULL, &length, NULL, 0) < 0)
2 -> arraysize(mib)

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode130
base/cpu.cc:130: if (sysctl(mib, 2, buffer, &length, NULL, 0) < 0)
buffer.get()
2 -> arraysize(mib)

https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode133
base/cpu.cc:133: base::Version kernel_version(buffer);
kernel_version is declared twice. Also Version is in the global namespace rather
than base.

[whunt, 2013-03-07 20:59:29]

On 2013/03/06 19:19:21, apatrick_chromium wrote:
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc
> File base/cpu.cc (right):
> 
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode18
> base/cpu.cc:18: #if defined (OS_MACOSX)
> nit: space between "defined" and "("
> 
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode19
> base/cpu.cc:19: #include "base/version.h"
> #include "base/memory/scoped_ptr.h"
> 
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode23
> base/cpu.cc:23: 
> #if defined(OS_WIN)
> #include <windows.h>
> #endif
> 
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode98
> base/cpu.cc:98: OSVERSIONINFO version_info;
> Consider OSVERSIONINFOEX to get service pack version. See below.
> 
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode100
> base/cpu.cc:100: if (!GetVersionEx(&version_info))
> You need to set version_info.dwOSVersionInfoSize to sizeof(version_info)
before
> calling this.
> 
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode103
> base/cpu.cc:103: // Windows 7 SP1 added support for AVX.
> This tests for Windows 7 or later rather than Windows 7 SP1 or later. There's
> code in base/win/windows_version.h to get the the service pack. Alternatively,
> version_info.wServicePackMajor >= 1 when Windows version is 6.1.
> 
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode111
> base/cpu.cc:111: if (!file_util::ReadFileToString("/proc/cpuinfo", &buffer))
> First argument should be of type base::FilePath.
> 
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode117
> base/cpu.cc:117: #elif  defined(OS_MACOSX)
> nit: two spaces between #elif and defined
> 
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode121
> base/cpu.cc:121: const static int mib[2] = {CTL_KERN, KERN_OSRELEASE};
> This is passed to sysctl so it can't be const.
> http://docs.oracle.com/cd/E19048-01/chorus5/806-7021/6jfu393f0/index.html
> 
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode123
> base/cpu.cc:123: if (sysctl(mib, 2, NULL, &length, NULL, 0) < 0)
> 2 -> arraysize(mib)
> 
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode130
> base/cpu.cc:130: if (sysctl(mib, 2, buffer, &length, NULL, 0) < 0)
> buffer.get()
> 2 -> arraysize(mib)
> 
> https://codereview.chromium.org/12511002/diff/9001/base/cpu.cc#newcode133
> base/cpu.cc:133: base::Version kernel_version(buffer);
> kernel_version is declared twice. Also Version is in the global namespace
rather
> than base.

I believe I have addressed all issues and the bots compile now.

[DaleCurtis, 2013-03-07 21:12:11]

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc#newcode115
base/cpu.cc:115: if
(!file_util::ReadFileToString(base::FilePath("/proc/cpuinfo"), &buffer))
As mentioned earlier, I'm pretty sure this will cause a sandbox violation.  Have
you tested this code on a Linux machine with AVX support? 
media/base/vector_math.cc and media/base/yuv_convert.cc both use this from the
renderer side (as well as in the GPU process I believe).

[whunt, 2013-03-07 21:17:08]

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc#newcode115
base/cpu.cc:115: if
(!file_util::ReadFileToString(base::FilePath("/proc/cpuinfo"), &buffer))
On 2013/03/07 21:12:11, DaleCurtis wrote:
> As mentioned earlier, I'm pretty sure this will cause a sandbox violation. 
Have
> you tested this code on a Linux machine with AVX support? 
> media/base/vector_math.cc and media/base/yuv_convert.cc both use this from the
> renderer side (as well as in the GPU process I believe).

With this patch, Chrome *appears* to run correctly under linux.  It launches, I
can surf.  I do get the following message at start-up but I'm not sure if it's
relevant or not: [17878:17878:0307/131402:ERROR:zygote_host_impl_linux.cc(146)]
Running without the SUID sandbox! See
https://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment for more
information on developing with the sandbox on.

[apatrick_chromium, 2013-03-07 21:22:47]

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc#newcode106
base/cpu.cc:106: return info->version() > base::win::VERSION_VISTA ||
This is checking for Windows 7 all versions or Vista SP1 and later. Comment says
Windows 7 SP1 and later. It looks like the Internet thinks the comment is right?
So the constants should be base::win::VERSION_WIN7 no? If
base::win::VERSION_VISTA is right then please update the comment.

[DaleCurtis, 2013-03-07 21:33:36]

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc#newcode115
base/cpu.cc:115: if
(!file_util::ReadFileToString(base::FilePath("/proc/cpuinfo"), &buffer))
On 2013/03/07 21:17:08, whunt wrote:
> On 2013/03/07 21:12:11, DaleCurtis wrote:
> > As mentioned earlier, I'm pretty sure this will cause a sandbox violation. 
> Have
> > you tested this code on a Linux machine with AVX support? 
> > media/base/vector_math.cc and media/base/yuv_convert.cc both use this from
the
> > renderer side (as well as in the GPU process I believe).
> 
> With this patch, Chrome *appears* to run correctly under linux.  It launches,
I
> can surf.  I do get the following message at start-up but I'm not sure if it's
> relevant or not:
[17878:17878:0307/131402:ERROR:zygote_host_impl_linux.cc(146)]
> Running without the SUID sandbox! See
> https://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment for more
> information on developing with the sandbox on.

Ah looks like it doesn't crash but instead just fails to open the file; so this
check will always be false when used in the renderer side on Linux.

If you can come up with a check which doesn't read a file that'd be best.

[whunt, 2013-03-07 21:49:03]

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc#newcode115
base/cpu.cc:115: if
(!file_util::ReadFileToString(base::FilePath("/proc/cpuinfo"), &buffer))
On 2013/03/07 21:33:36, DaleCurtis wrote:
> On 2013/03/07 21:17:08, whunt wrote:
> > On 2013/03/07 21:12:11, DaleCurtis wrote:
> > > As mentioned earlier, I'm pretty sure this will cause a sandbox violation.

> > Have
> > > you tested this code on a Linux machine with AVX support? 
> > > media/base/vector_math.cc and media/base/yuv_convert.cc both use this from
> the
> > > renderer side (as well as in the GPU process I believe).
> > 
> > With this patch, Chrome *appears* to run correctly under linux.  It
launches,
> I
> > can surf.  I do get the following message at start-up but I'm not sure if
it's
> > relevant or not:
> [17878:17878:0307/131402:ERROR:zygote_host_impl_linux.cc(146)]
> > Running without the SUID sandbox! See
> > https://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment for more
> > information on developing with the sandbox on.
> 
> Ah looks like it doesn't crash but instead just fails to open the file; so
this
> check will always be false when used in the renderer side on Linux.
> 
> If you can come up with a check which doesn't read a file that'd be best.

I am neither very knowledgeable about linux or the sandbox.  linux_util.h
doesn't have an interface for getting the kernel version (we need kernel version
2.6.30).  I assume opening /proc/version or /proc/sys/kernel/osrelease will have
the same problem.  Do you know who I could ask about this?

[DaleCurtis, 2013-03-07 21:57:43]

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc#newcode115
base/cpu.cc:115: if
(!file_util::ReadFileToString(base::FilePath("/proc/cpuinfo"), &buffer))
On 2013/03/07 21:49:03, whunt wrote:
> On 2013/03/07 21:33:36, DaleCurtis wrote:
> > On 2013/03/07 21:17:08, whunt wrote:
> > > On 2013/03/07 21:12:11, DaleCurtis wrote:
> > > > As mentioned earlier, I'm pretty sure this will cause a sandbox
violation.
> 
> > > Have
> > > > you tested this code on a Linux machine with AVX support? 
> > > > media/base/vector_math.cc and media/base/yuv_convert.cc both use this
from
> > the
> > > > renderer side (as well as in the GPU process I believe).
> > > 
> > > With this patch, Chrome *appears* to run correctly under linux.  It
> launches,
> > I
> > > can surf.  I do get the following message at start-up but I'm not sure if
> it's
> > > relevant or not:
> > [17878:17878:0307/131402:ERROR:zygote_host_impl_linux.cc(146)]
> > > Running without the SUID sandbox! See
> > > https://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment for
more
> > > information on developing with the sandbox on.
> > 
> > Ah looks like it doesn't crash but instead just fails to open the file; so
> this
> > check will always be false when used in the renderer side on Linux.
> > 
> > If you can come up with a check which doesn't read a file that'd be best.
> 
> I am neither very knowledgeable about linux or the sandbox.  linux_util.h
> doesn't have an interface for getting the kernel version (we need kernel
version
> 2.6.30).  I assume opening /proc/version or /proc/sys/kernel/osrelease will
have
> the same problem.  Do you know who I could ask about this?

Looks like you can use the xgetbv intrinsic once the CPUID indicates avx is
available.  I believe this is platform independent, so you don't need to use any
of these other platform specific defines.  Take a look at these instances
already in the code base:

https://code.google.com/p/chromium/codesearch#search/&q=xgetbv&sq=package:chr...

[DaleCurtis, 2013-03-07 22:15:46]

Looks like clang trunk supports this, but probably not Xcode clang or gcc so
you'll need to emit the raw bytecode for Linux/Mac.  This looks reasonable:

http://pastebin.com/DK9xvLRL

On Windows you can include immintrin.h and use _xgetbv(), more info here:

http://insufficientlycomplicated.wordpress.com/2011/11/07/detecting-intel-adv...

[whunt, 2013-03-07 23:28:54]

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/13001/base/cpu.cc#newcode115
base/cpu.cc:115: if
(!file_util::ReadFileToString(base::FilePath("/proc/cpuinfo"), &buffer))
On 2013/03/07 21:57:43, DaleCurtis wrote:
> On 2013/03/07 21:49:03, whunt wrote:
> > On 2013/03/07 21:33:36, DaleCurtis wrote:
> > > On 2013/03/07 21:17:08, whunt wrote:
> > > > On 2013/03/07 21:12:11, DaleCurtis wrote:
> > > > > As mentioned earlier, I'm pretty sure this will cause a sandbox
> violation.
> > 
> > > > Have
> > > > > you tested this code on a Linux machine with AVX support? 
> > > > > media/base/vector_math.cc and media/base/yuv_convert.cc both use this
> from
> > > the
> > > > > renderer side (as well as in the GPU process I believe).
> > > > 
> > > > With this patch, Chrome *appears* to run correctly under linux.  It
> > launches,
> > > I
> > > > can surf.  I do get the following message at start-up but I'm not sure
if
> > it's
> > > > relevant or not:
> > > [17878:17878:0307/131402:ERROR:zygote_host_impl_linux.cc(146)]
> > > > Running without the SUID sandbox! See
> > > > https://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment for
> more
> > > > information on developing with the sandbox on.
> > > 
> > > Ah looks like it doesn't crash but instead just fails to open the file; so
> > this
> > > check will always be false when used in the renderer side on Linux.
> > > 
> > > If you can come up with a check which doesn't read a file that'd be best.
> > 
> > I am neither very knowledgeable about linux or the sandbox.  linux_util.h
> > doesn't have an interface for getting the kernel version (we need kernel
> version
> > 2.6.30).  I assume opening /proc/version or /proc/sys/kernel/osrelease will
> have
> > the same problem.  Do you know who I could ask about this?
> 
> Looks like you can use the xgetbv intrinsic once the CPUID indicates avx is
> available.  I believe this is platform independent, so you don't need to use
any
> of these other platform specific defines.  Take a look at these instances
> already in the code base:
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=xgetbv&sq=package:chr...

awesome suggestion, done

[DaleCurtis, 2013-03-07 23:37:15]

https://codereview.chromium.org/12511002/diff/24001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/24001/base/cpu.cc#newcode132
base/cpu.cc:132: has_avx_ = (cpu_info[2] & 0x10000000) != 0 && (_xgetbv(0) & 6)
== 6;
I think this needs #include <immintrin.h> on Windows and that only works on
64-bit according to this bug:

https://code.google.com/p/libyuv/issues/detail?id=106

Perhaps that's fixed now? Have you done a Windows build with this?

[whunt, 2013-03-07 23:45:40]

On 2013/03/07 23:37:15, DaleCurtis wrote:
> https://codereview.chromium.org/12511002/diff/24001/base/cpu.cc
> File base/cpu.cc (right):
> 
> https://codereview.chromium.org/12511002/diff/24001/base/cpu.cc#newcode132
> base/cpu.cc:132: has_avx_ = (cpu_info[2] & 0x10000000) != 0 && (_xgetbv(0) &
6)
> == 6;
> I think this needs #include <immintrin.h> on Windows and that only works on
> 64-bit according to this bug:
> 
> https://code.google.com/p/libyuv/issues/detail?id=106
> 
> Perhaps that's fixed now? Have you done a Windows build with this?

intrin.h should be sufficient.  I'll check on the 32-64 bit issue.

[apatrick_chromium, 2013-03-08 00:39:41]

https://codereview.chromium.org/12511002/diff/24001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/24001/base/cpu.cc#newcode132
base/cpu.cc:132: has_avx_ = (cpu_info[2] & 0x10000000) != 0 && (_xgetbv(0) & 6)
== 6;
This says that it need Visual Studio 2010 SP1, which is a Chromium build
prerequisite.

http://insufficientlycomplicated.wordpress.com/2011/11/07/detecting-intel-adv...

It also suggests the need to check for XSAVE/XRSTOR support, bit 27 of
cpu_info[2].

[whunt, 2013-03-08 18:30:31]

https://codereview.chromium.org/12511002/diff/24001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/12511002/diff/24001/base/cpu.cc#newcode132
base/cpu.cc:132: has_avx_ = (cpu_info[2] & 0x10000000) != 0 && (_xgetbv(0) & 6)
== 6;
XSAVE/XRSTOR are required for SSE to work at all and are supported by all OSes
we support.

On 2013/03/08 00:39:41, apatrick_chromium wrote:
> This says that it need Visual Studio 2010 SP1, which is a Chromium build
> prerequisite.
> 
>
http://insufficientlycomplicated.wordpress.com/2011/11/07/detecting-intel-adv...
> 
> It also suggests the need to check for XSAVE/XRSTOR support, bit 27 of
> cpu_info[2].

https://codereview.chromium.org/12511002/diff/24001/base/cpu.cc#newcode132
base/cpu.cc:132: has_avx_ = (cpu_info[2] & 0x10000000) != 0 && (_xgetbv(0) & 6)
== 6;
<intrin.h> should include <immintrin.h>
I've only run the bots with it which I believe are 32 bit (I'm not aware of a
64bit windows build).

On 2013/03/07 23:37:15, DaleCurtis wrote:
> I think this needs #include <immintrin.h> on Windows and that only works on
> 64-bit according to this bug:
> 
> https://code.google.com/p/libyuv/issues/detail?id=106
> 
> Perhaps that's fixed now? Have you done a Windows build with this?

[DaleCurtis, 2013-03-08 18:37:51]

lgtm

[commit-bot: I haz the power, 2013-03-08 18:57:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whunt@chromium.org/12511002/24001

[commit-bot: I haz the power, 2013-03-08 18:57:57]

Presubmit check for 12511002-24001 failed and returned exit status 1.

INFO:root:Found 1 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/base/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    base/cpu.cc

Presubmit checks took 1.2s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[apatrick_chromium, 2013-03-08 23:16:19]

You need brettw for owners LGTM. Since he's out today, I suggest describing how
this was tested on all platforms concerned. I think he might want to know that.

[whunt, 2013-03-13 18:47:45]

On 2013/03/08 23:16:19, apatrick_chromium wrote:
> You need brettw for owners LGTM. Since he's out today, I suggest describing
how
> this was tested on all platforms concerned. I think he might want to know
that.

brettw, do you have time to take a look at this?

[DaleCurtis, 2013-07-10 18:32:33]

Still planning to land this? Our AVX metrics are off without it.

[whunt_google.com, 2013-07-10 18:49:56]

It needed brettw's LGTM.  I'm no longer on Chrome team.

-Warren


On Wed, Jul 10, 2013 at 11:32 AM, <dalecurtis@chromium.org> wrote:

> Still planning to land this? Our AVX metrics are off without it.
>
>
https://codereview.chromium.**org/12511002/<https://codereview.chromium.org/1...
>

[brettw, 2013-07-11 18:11:45]

lgtm

[commit-bot: I haz the power, 2013-07-12 18:18:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whunt@chromium.org/12511002/24001

[commit-bot: I haz the power, 2013-07-12 18:18:20]

Failed to apply patch for base/cpu.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file base/cpu.cc
  Hunk #1 FAILED at 32.
  Hunk #2 succeeded at 81 (offset 1 line).
  Hunk #3 succeeded at 129 (offset 1 line).
  1 out of 3 hunks FAILED -- saving rejects to file base/cpu.cc.rej

Patch:       base/cpu.cc
Index: base/cpu.cc
diff --git a/base/cpu.cc b/base/cpu.cc
index
d6976bab33b316aed50e3a133edf1a7e8a060a97..74513739f121ebc348e48a3b8f4cd0ed0cfefcf3
100644
--- a/base/cpu.cc
+++ b/base/cpu.cc
@@ -32,6 +32,7 @@ CPU::CPU()
     has_ssse3_(false),
     has_sse41_(false),
     has_sse42_(false),
+    has_avx_(false),
     cpu_vendor_("unknown") {
   Initialize();
 }
@@ -80,6 +81,17 @@ void __cpuidex(int cpu_info[4], int info_type, int
info_index) {
 }
 
 #endif
+
+unsigned long long _xgetbv(unsigned int xcr) {
+  unsigned int eax, edx;
+  __asm__ volatile (
+    ".byte 0x0f, 0x01, 0xd0"
+    : "=a"(eax), "=d"(edx)
+    : "c" (xcr)
+  );
+  return static_cast<unsigned long long>(eax) ^
+         static_cast<unsigned long long>(edx) << 32;
+}
 #endif  // _MSC_VER
 #endif  // ARCH_CPU_X86_FAMILY
 
@@ -117,7 +129,7 @@ void CPU::Initialize() {
     has_ssse3_ = (cpu_info[2] & 0x00000200) != 0;
     has_sse41_ = (cpu_info[2] & 0x00080000) != 0;
     has_sse42_ = (cpu_info[2] & 0x00100000) != 0;
-    has_avx_ = (cpu_info[2] & 0x10000000) != 0;
+    has_avx_ = (cpu_info[2] & 0x10000000) != 0 && (_xgetbv(0) & 6) == 6;
   }
 
   // Get the brand string of the cpu.

[DaleCurtis, 2013-08-09 20:59:46]

Rebased and landing for whunt@ here https://codereview.chromium.org/22354004/ --
Closing this CL.