Fix arm/LTO build by moving neon asm files to intrinsics target.

Fix arm/LTO build by moving neon asm files to intrinsics target.

We currently build certain asm files with support for neon instructions by
passing -Wa,-mfpu=neon when building the libvpx target. This won't work under
LTO because of unused argument errors when building the non-asm files. We
cannot use -mfpu=neon here because neon is an optional feature of libvpx
target on Android if use_neon==0, and we presumably don't want the rest of
the files in the target to use neon instructions.

It looks like the right thing to do here is to move the asm files to the
libvpx_intrinsics_neon target, which is already set up to build everything
for neon with -mfpu=neon.

BUG=407544
R=johannkoenig@google.com, thakis@chromium.org, tomfinegan@chromium.org

Committed: https://chromium.googlesource.com/chromium/deps/libvpx/+/7f648118deef202a106d6ac8625bbe3b84369229

[fgalligan1, 2015-04-15 06:13:18]

fgalligan@chromium.org changed reviewers:
+ johannkoenig@chromium.org

[Johann, 2015-04-15 15:38:44]

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

[Johann, 2015-04-15 15:38:45]

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

https://codereview.chromium.org/1085023004/diff/1/generate_gypi.sh#newcode159
generate_gypi.sh:159: local neon_sources=$(echo "$file_list" | grep
'_neon\.c$\|\.asm$')
while *most* of the neon code is named neon_asm.asm, some of it is still
neon.asm. we can change that, it's just more intermediate steps before fixing.
alternatively, if this would search for 'neon' anywhere in the file name + .asm
extension, that would potentially simplify things.

or the neon asm could be broken out to it's own target. i'm not sure if the
assembler will get the cflag for mfpu=neon, or if it specifically needs the one
targeted at the assembler.

https://codereview.chromium.org/1085023004/diff/1/generate_gypi.sh#newcode236
generate_gypi.sh:236: # they need the -mfpu=neon flag.
is it sufficient to give them -mfpu=neon or do they require -Wa,-mfpu=neon?

https://codereview.chromium.org/1085023004/diff/1/libvpx_srcs_arm_neon_cpu_de...
File libvpx_srcs_arm_neon_cpu_detect_intrinsics.gypi (right):

https://codereview.chromium.org/1085023004/diff/1/libvpx_srcs_arm_neon_cpu_de...
libvpx_srcs_arm_neon_cpu_detect_intrinsics.gypi:35:
'<(libvpx_source)/vp8/common/arm/armv6/vp8_variance_halfpixvar16x16_v_armv6.asm',
although we managed to transition all the vp8 neon to intrinsics, not all the
vp9 code made the cut. while i don't think there will be an issue with
specifying -mfpu=neon on the armv6 files (there is significantly less leeway for
the compiler to surreptitiously insert unintended instructions), it is a bit
strange.

[pcc1, 2015-04-15 18:50:34]

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

https://codereview.chromium.org/1085023004/diff/1/generate_gypi.sh#newcode159
generate_gypi.sh:159: local neon_sources=$(echo "$file_list" | grep
'_neon\.c$\|\.asm$')
On 2015/04/15 15:38:45, Johann wrote:
> while *most* of the neon code is named neon_asm.asm, some of it is still
> neon.asm. we can change that, it's just more intermediate steps before fixing.
> alternatively, if this would search for 'neon' anywhere in the file name +
.asm
> extension, that would potentially simplify things.

Makes sense, but this regex just grabs the asm files out of the list that was
already computed by convert_srcs_to_project_files below. I've updated the regex
there.

> or the neon asm could be broken out to it's own target. i'm not sure if the
> assembler will get the cflag for mfpu=neon, or if it specifically needs the
one
> targeted at the assembler.

The compiler driver will pass the -mfpu=neon flag to the assembler.

https://codereview.chromium.org/1085023004/diff/1/generate_gypi.sh#newcode236
generate_gypi.sh:236: # they need the -mfpu=neon flag.
On 2015/04/15 15:38:45, Johann wrote:
> is it sufficient to give them -mfpu=neon or do they require -Wa,-mfpu=neon?

The former. The compiler driver will pass the -mfpu=neon flag to the assembler.

https://codereview.chromium.org/1085023004/diff/1/libvpx_srcs_arm_neon_cpu_de...
File libvpx_srcs_arm_neon_cpu_detect_intrinsics.gypi (right):

https://codereview.chromium.org/1085023004/diff/1/libvpx_srcs_arm_neon_cpu_de...
libvpx_srcs_arm_neon_cpu_detect_intrinsics.gypi:35:
'<(libvpx_source)/vp8/common/arm/armv6/vp8_variance_halfpixvar16x16_v_armv6.asm',
On 2015/04/15 15:38:45, Johann wrote:
> although we managed to transition all the vp8 neon to intrinsics, not all the
> vp9 code made the cut. while i don't think there will be an issue with
> specifying -mfpu=neon on the armv6 files (there is significantly less leeway
for
> the compiler to surreptitiously insert unintended instructions), it is a bit
> strange.

Moved as a result of the regex change.

FWIW, it looks like we are already doing this in the GN build (grep for
libvpx_srcs_arm_neon_cpu_detect_assembly in BUILD.gn and libvpx_srcs.gni). That
should probably be fixed as well. On the other hand, I'm not too familiar with
arm, but I'd be surprised if selecting a different FPU caused the assembler to
emit different instructions for the same source file (rather than changing the
set of instructions to allow).

In any case, it seems more useful as a guard against accidentally using neon
instructions in the non-neon asm files.

[Johann, 2015-04-15 19:09:06]

LGTM

Yeah, would be nice to fix the BUILD.gn too, but I'm not sure if that is that
way because something about it is fragile. If you don't want to muck, that's
fine. I would hope the assembler wouldn't bugger it, but I've seen the compiler
do all sorts of fun things.

https://codereview.chromium.org/1085023004/diff/20001/ads2gas.gypi
File ads2gas.gypi (right):

https://codereview.chromium.org/1085023004/diff/20001/ads2gas.gypi#newcode1
ads2gas.gypi:1: # Copyright (c) 2015 The Chromium Authors. All rights reserved.
This is just a refactoring?

[pcc1, 2015-04-16 22:00:09]

Nico, do the gyp changes look good to you?

https://codereview.chromium.org/1085023004/diff/20001/ads2gas.gypi
File ads2gas.gypi (right):

https://codereview.chromium.org/1085023004/diff/20001/ads2gas.gypi#newcode1
ads2gas.gypi:1: # Copyright (c) 2015 The Chromium Authors. All rights reserved.
On 2015/04/15 19:09:06, Johann wrote:
> This is just a refactoring?

Not any more it isn't :)

[Nico, 2015-04-24 21:43:23]

rs-lgtm ; Johann knows this code much better than I do :-)

https://codereview.chromium.org/1085023004/diff/40001/ads2gas.gypi
File ads2gas.gypi (right):

https://codereview.chromium.org/1085023004/diff/40001/ads2gas.gypi#newcode4
ads2gas.gypi:4: {
gypi files should have some "how to use" comment at the top (see e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/build/protoc.gypi&...)

[pcc1, 2015-04-27 21:14:23]

On 2015/04/15 19:09:06, Johann wrote:
> LGTM
> 
> Yeah, would be nice to fix the BUILD.gn too, but I'm not sure if that is that
> way because something about it is fragile. If you don't want to muck, that's
> fine. I would hope the assembler wouldn't bugger it, but I've seen the
compiler
> do all sorts of fun things.
> 
> https://codereview.chromium.org/1085023004/diff/20001/ads2gas.gypi
> File ads2gas.gypi (right):
> 
> https://codereview.chromium.org/1085023004/diff/20001/ads2gas.gypi#newcode1
> ads2gas.gypi:1: # Copyright (c) 2015 The Chromium Authors. All rights
reserved.
> This is just a refactoring?

Johann: please land, still can't do it myself :/ Thanks!

[Johann, 2015-04-27 21:20:12]

On 2015/04/27 21:14:23, pcc1 wrote:
> Johann: please land, still can't do it myself :/ Thanks!

I put in a request last week to move libvpx out of DEPS :)

[Johann, 2015-04-27 21:20:50]

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