Use the correct AVX flags for Windows builds

Use the correct AVX flags for Windows builds

R=thakis@chromium.org

BUG=451290
Committed: 8597bfe7e1a9c70f5a7ee785742b017a63832c5a

[Johann, 2015-01-23 02:29:20]

Sorry for the breakage. Do you think this CL will fix it? Otherwise I'll muck
about elsewhere in the code.

[Nico, 2015-01-23 02:57:12]

Thanks!

https://codereview.chromium.org/866273003/diff/1/generate_gypi.sh
File generate_gypi.sh (right):

https://codereview.chromium.org/866273003/diff/1/generate_gypi.sh#newcode120
generate_gypi.sh:120: if [[ $4 == avx* ]]; then
Shouldn't this block cover that already?

https://codereview.chromium.org/866273003/diff/1/generate_gypi.sh#newcode123
generate_gypi.sh:123: echo "          'EnableEnhancedInstructionSet': '3', #
/arch:AVX" >> "$2"
I think the problem here is that this unconditionally passes '3'. First, gyp
needs to learn which number corresponds to AVX2 here
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
(I'll take care of that), and then this needs to use '3' for AVX and whatever
the number for AVX2 is for AVX2. Then everything should work.

[Johann, 2015-01-23 03:26:35]

I'm not sure the exact number of ChromeOS devices with avx2 but it first arrived
in haswell. Quick searches suggest it started showing up in fall 2013 in the
acer c720 and there was another bunch based on the core i3 in fall 2014.

https://codereview.chromium.org/866273003/diff/1/generate_gypi.sh
File generate_gypi.sh (right):

https://codereview.chromium.org/866273003/diff/1/generate_gypi.sh#newcode120
generate_gypi.sh:120: if [[ $4 == avx* ]]; then
On 2015/01/23 02:57:12, Nico wrote:
> Shouldn't this block cover that already?

Well, this block covers using "EnableEnhancedInstructionSet" which I wasn't sure
was used by clang. Rather than duplicate the code below and add a condition for
clang==1 and AdditionalOptions -mavx2, it seemed easiest to add the avx2 flag
below.

It also tops out at AVX, but msvc doesn't seem to care and still accepts the
avx2 instructions (see conversation here:
https://codereview.chromium.org/817293008/)

https://codereview.chromium.org/866273003/diff/1/generate_gypi.sh#newcode123
generate_gypi.sh:123: echo "          'EnableEnhancedInstructionSet': '3', #
/arch:AVX" >> "$2"
On 2015/01/23 02:57:12, Nico wrote:
> I think the problem here is that this unconditionally passes '3'. First, gyp
> needs to learn which number corresponds to AVX2 here
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
> (I'll take care of that), and then this needs to use '3' for AVX and whatever
> the number for AVX2 is for AVX2. Then everything should work.

We only currently have avx2 code. We can switch the if statement to == avx2 if
that is cleaner. See the other comment for avx/avx2.

[Nico, 2015-01-23 19:14:29]

https://codereview.chromium.org/866273003/diff/1/generate_gypi.sh
File generate_gypi.sh (right):

https://codereview.chromium.org/866273003/diff/1/generate_gypi.sh#newcode120
generate_gypi.sh:120: if [[ $4 == avx* ]]; then
On 2015/01/23 03:26:35, Johann wrote:
> On 2015/01/23 02:57:12, Nico wrote:
> > Shouldn't this block cover that already?
> 
> Well, this block covers using "EnableEnhancedInstructionSet" which I wasn't
sure
> was used by clang. Rather than duplicate the code below and add a condition
for
> clang==1 and AdditionalOptions -mavx2, it seemed easiest to add the avx2 flag
> below.

On Windows, we use clang through clang-cl, a compiler driver that aims to be
commandline-compatible with cl.exe (msvc's compiler). In theory, clang will use
all the Windows settings from gyp. (In practice, this is true too.)

> It also tops out at AVX, but msvc doesn't seem to care and still accepts the
> avx2 instructions (see conversation here:
> https://codereview.chromium.org/817293008/)

You want /arch:AVX2, in gn too. I think the linked-to patch isn't right, if you
want AVX2.

One issue is that gyp didn't expose this. I fixed this in
https://codereview.chromium.org/872643002/ , and once
https://codereview.chromium.org/873793002/ is in, you can say '5' for AVX2 in
gyp files.

https://codereview.chromium.org/866273003/diff/1/generate_gypi.sh#newcode123
generate_gypi.sh:123: echo "          'EnableEnhancedInstructionSet': '3', #
/arch:AVX" >> "$2"
On 2015/01/23 03:26:35, Johann wrote:
> On 2015/01/23 02:57:12, Nico wrote:
> > I think the problem here is that this unconditionally passes '3'. First, gyp
> > needs to learn which number corresponds to AVX2 here
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
> > (I'll take care of that), and then this needs to use '3' for AVX and
whatever
> > the number for AVX2 is for AVX2. Then everything should work.
> 
> We only currently have avx2 code. We can switch the if statement to == avx2 if
> that is cleaner. See the other comment for avx/avx2.

If this is only for avx2, then I'd change the statement to == avx2. If you
change the EnableEnhancedInstructionSet line to

  echo "          'EnableEnhancedInstructionSet': '5', # /arch:AVX2" >> "$2"

then everything works fine with clang – I just tested that locally.

(This will only work once https://codereview.chromium.org/873793002/ has
landed.)

[Johann, 2015-01-23 19:19:51]

On 2015/01/23 19:14:29, Nico wrote:
> On 2015/01/23 03:26:35, Johann wrote:
> > On 2015/01/23 02:57:12, Nico wrote:
> > > Shouldn't this block cover that already?
> > 
> > Well, this block covers using "EnableEnhancedInstructionSet" which I wasn't
> sure
> > was used by clang. Rather than duplicate the code below and add a condition
> for
> > clang==1 and AdditionalOptions -mavx2, it seemed easiest to add the avx2
flag
> > below.
> 
> On Windows, we use clang through clang-cl, a compiler driver that aims to be
> commandline-compatible with cl.exe (msvc's compiler). In theory, clang will
use
> all the Windows settings from gyp. (In practice, this is true too.)

Ah, that makes sense.
 
> > It also tops out at AVX, but msvc doesn't seem to care and still accepts the
> > avx2 instructions (see conversation here:
> > https://codereview.chromium.org/817293008/)
> 
> You want /arch:AVX2, in gn too. I think the linked-to patch isn't right, if
you
> want AVX2.

OK, will update

> If this is only for avx2, then I'd change the statement to == avx2. If you
> change the EnableEnhancedInstructionSet line to
> 
>   echo "          'EnableEnhancedInstructionSet': '5', # /arch:AVX2" >> "$2"
> 
> then everything works fine with clang – I just tested that locally.

Will get this too
 
> (This will only work once https://codereview.chromium.org/873793002/ has
> landed.)

Unfortunately the code broke on winxp (may be cpu-detection related) so I'll
probably make an update, remove the avx2 blocks, and then try again when we
figure out what's up with xp. if you happen to have such a device laying around,
please let me know.

[Nico, 2015-01-23 19:21:34]

On 2015/01/23 19:19:51, Johann wrote:
> On 2015/01/23 19:14:29, Nico wrote:
> > On 2015/01/23 03:26:35, Johann wrote:
> > > On 2015/01/23 02:57:12, Nico wrote:
> > > > Shouldn't this block cover that already?
> > > 
> > > Well, this block covers using "EnableEnhancedInstructionSet" which I
wasn't
> > sure
> > > was used by clang. Rather than duplicate the code below and add a
condition
> > for
> > > clang==1 and AdditionalOptions -mavx2, it seemed easiest to add the avx2
> flag
> > > below.
> > 
> > On Windows, we use clang through clang-cl, a compiler driver that aims to be
> > commandline-compatible with cl.exe (msvc's compiler). In theory, clang will
> use
> > all the Windows settings from gyp. (In practice, this is true too.)
> 
> Ah, that makes sense.
>  
> > > It also tops out at AVX, but msvc doesn't seem to care and still accepts
the
> > > avx2 instructions (see conversation here:
> > > https://codereview.chromium.org/817293008/)
> > 
> > You want /arch:AVX2, in gn too. I think the linked-to patch isn't right, if
> you
> > want AVX2.
> 
> OK, will update
> 
> > If this is only for avx2, then I'd change the statement to == avx2. If you
> > change the EnableEnhancedInstructionSet line to
> > 
> >   echo "          'EnableEnhancedInstructionSet': '5', # /arch:AVX2" >> "$2"
> > 
> > then everything works fine with clang – I just tested that locally.
> 
> Will get this too
>  
> > (This will only work once https://codereview.chromium.org/873793002/ has
> > landed.)
> 
> Unfortunately the code broke on winxp (may be cpu-detection related) so I'll
> probably make an update, remove the avx2 blocks, and then try again when we
> figure out what's up with xp. if you happen to have such a device laying
around,
> please let me know.

What other targets do, and what you'll need to do if you aren't yet, is to put
all your avx2 code into their own translation unit (looks like you're doing this
already), and then before calling any of this avx2 code, check that a) the
processor supports avx2 b) check that the os is at least windows 7. Then things
should work in theory.

[Johann, 2015-01-23 19:25:38]

> What other targets do, and what you'll need to do if you aren't yet, is to put
> all your avx2 code into their own translation unit (looks like you're doing
this
> already), and then before calling any of this avx2 code, check that a) the
> processor supports avx2 b) check that the os is at least windows 7. Then
things
> should work in theory.

Yup and yup. The code builds for XP, but for some reason it just hangs in some
tests. Working theory is that the cpu detection is seeing CPU support, but for
some reason the OS check is failing and reporting support as well when it
shouldn't. Our detection is (in theory) more complete than V8's (see last
comment https://codereview.chromium.org/860003002/) and was just updated because
someone was running BSD VMs with avx turned off in the kernel
https://code.google.com/p/webm/issues/detail?id=790

[Johann, 2015-01-23 21:23:48]

this patch uses the new '5' value

[Nico, 2015-01-23 21:25:37]

lgtm, thanks!

[Johann, 2015-01-23 21:28:33]

Committed patchset #3 (id:40001) manually as
8597bfe7e1a9c70f5a7ee785742b017a63832c5a (presubmit successful).