Add a [rtc_]build_with_neon variable to unify conditions.

Add a [rtc_]build_with_neon variable to unify conditions.

Also consolidate ARM options for gn in an arm_neon_config.

R=jridges@masque.com, kjellander@webrtc.org, zhongwei.yao@chromium.org

Committed: https://chromium.googlesource.com/external/webrtc/+/ac4234ccfc964208163d6152977cc1d015448bbb

[Andrew MacDonald, 2015-06-17 05:38:14]

andrew@webrtc.org changed reviewers:
+ jridges@masque.com, kjellander@webrtc.org, zhongwei.yao@chromium.org

[Andrew MacDonald, 2015-06-17 05:38:15]

Certainly not ready for commit, but do we want to add a variable like this? It
should simplify the conditions elsewhere. See here for a related problem:
https://codereview.webrtc.org/1187833002/

[kjellander_webrtc, 2015-06-17 06:00:55]

very nice, I think this is a big improvement in the ARM and Neon confusion.
lgtm

[Mostyn Bramley-Moore, 2015-06-17 08:27:47]

This patch works for my non-neon armv7 test case, if I change the two conditions
with arm_version>=7 in webrtc/common_audio/common_audio.gyp to be simply
'build_arm_neon==1'.

[jridges, 2015-06-17 15:40:33]

Not being familiar with the build system, I don't have much to say about this
except that I would rather see the variables named "build_with_neon" rather than
"build_arm_neon". I think it's important to decouple NEON from ARM, as there
could be cases where you would use the NEON intrinsics on an x86 Android
platform using Intel's NEON emulation header (for instance, if no-one wants to
go to the trouble of implementing SSE code for the NSX or AECM modules).

[Andrew MacDonald, 2015-06-17 17:42:39]

On 2015/06/17 15:40:33, jridges wrote:
> Not being familiar with the build system, I don't have much to say about this
> except that I would rather see the variables named "build_with_neon" rather
than
> "build_arm_neon". I think it's important to decouple NEON from ARM, as there
> could be cases where you would use the NEON intrinsics on an x86 Android
> platform using Intel's NEON emulation header (for instance, if no-one wants to
> go to the trouble of implementing SSE code for the NSX or AECM modules).

Makes sense. I'll change the name, but keep the same conditions, as we assume
NEON implies ARM everywhere now.

[Andrew MacDonald, 2015-06-17 17:44:16]

Ready for review. PTAL.

https://codereview.webrtc.org/1181373004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (left):

https://codereview.webrtc.org/1181373004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:584: sources += [
"codecs/isac/fix/source/lattice_c.c" ]
I think adding this to the primary source list and removing it as appropriate
with each architecture is more clear. The gn file does it this way.

https://codereview.webrtc.org/1181373004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/1181373004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:544: if (current_cpu == "arm" &&
arm_version >= 7) {
I know the armv6 file below doesn't require arm_version >= 7, but I honestly
don't think it's worth adding another condition for it.

https://codereview.webrtc.org/1181373004/diff/40001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/1181373004/diff/40001/webrtc/BUILD.gn#newcode168
webrtc/BUILD.gn:168: config("arm_neon_config") {
kjellander: I'm not super familiar with this stuff. Do you see any reason this
won't work?

[jridges, 2015-06-17 18:45:09]

On 2015/06/17 17:42:39, andrew wrote:
> On 2015/06/17 15:40:33, jridges wrote:
> > Not being familiar with the build system, I don't have much to say about
this
> > except that I would rather see the variables named "build_with_neon" rather
> than
> > "build_arm_neon". I think it's important to decouple NEON from ARM, as there
> > could be cases where you would use the NEON intrinsics on an x86 Android
> > platform using Intel's NEON emulation header (for instance, if no-one wants
to
> > go to the trouble of implementing SSE code for the NSX or AECM modules).
> 
> Makes sense. I'll change the name, but keep the same conditions, as we assume
> NEON implies ARM everywhere now.

Sounds good. LGTM

[jridges, 2015-06-17 18:46:51]

https://codereview.webrtc.org/1181373004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/1181373004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:544: if (current_cpu == "arm" &&
arm_version >= 7) {
On 2015/06/17 17:44:16, andrew wrote:
> I know the armv6 file below doesn't require arm_version >= 7, but I honestly
> don't think it's worth adding another condition for it.

I agree, especially since the plan is to get rid of the .S files eventually
anyway.

[Andrew MacDonald, 2015-06-17 22:46:22]

New patchset up with a few fixes.

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/common.gypi
File webrtc/build/common.gypi (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/common.gypi#...
webrtc/build/common.gypi:160: 'build_with_neon%': 1,
gyp wasn't picking up the variable changes in the other scopes, so let's just do
it at one place here. Matches gn better anyway.

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/common_audio/BUILD.gn
File webrtc/common_audio/BUILD.gn (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/common_audio/BUILD...
webrtc/common_audio/BUILD.gn:223: if (current_cpu != "arm64") {
It unfortunately isn't possible to extract these options to a config. configs
only allow additive changes and can't affect other configs.

[zhongwei, 2015-06-18 02:35:07]

Nice patch! LGTM

[Andrew MacDonald, 2015-06-18 03:13:37]

Henrik, have a quick look at the latest changes.

[kjellander_webrtc, 2015-06-18 09:06:23]

lgtm to unblock you but please fix some of the comments.

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/arm_neon.gypi
File webrtc/build/arm_neon.gypi (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/arm_neon.gyp...
webrtc/build/arm_neon.gypi:33: # Disable LTO due to compiler bug.
Please add a TODO + reference to the bug.
Maybe even create another bug in the WebRTC tracker to cover re-enabling this?

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/common.gypi
File webrtc/build/common.gypi (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/common.gypi#...
webrtc/build/common.gypi:124: # Determines whether NEON code will be built. It
might not be selected at
I assume you mean "WebRTC doesn't support selecting it at runtime" ? Please
phrase it that way since it's unclear to me in the current comment.

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/webrtc.gni
File webrtc/build/webrtc.gni (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/webrtc.gni#n...
webrtc/build/webrtc.gni:108: # Determines whether NEON code will be built. It
might not be selected at
Same comment update as in common.gypi...

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:549: sources -= [
I don't like the minus-entries for source listings, but I guess it's easier to
read this way.

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:588: if (current_cpu != "arm64" ||
!is_clang) {
This was fixed in Clang r235243 and we're currently at r238013
(https://chromium.googlesource.com/chromium/src/+/441009c0b2454f93adf9d30a42ea...)

I posted in the bug asking for re-enabling this (no need in this CL).

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:603: # Disable LTO due to compiler bug.
Please link to the bug etc, similar to my previous comment.

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/BUILD.gn (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/BUILD.gn:258: # Disable LTO due to compiler bug.
Add link to the bug as previous comment.

[zhongwei, 2015-06-19 08:21:03]

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:588: if (current_cpu != "arm64" ||
!is_clang) {
On 2015/06/18 09:06:23, kjellander (OOO til Aug 31) wrote:
> This was fixed in Clang r235243 and we're currently at r238013
>
(https://chromium.googlesource.com/chromium/src/+/441009c0b2454f93adf9d30a42ea...)
> 
> I posted in the bug asking for re-enabling this (no need in this CL).

I've posted the patch at: https://codereview.webrtc.org/1194773002/

[Andrew MacDonald, 2015-06-25 01:09:09]

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/arm_neon.gypi
File webrtc/build/arm_neon.gypi (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/arm_neon.gyp...
webrtc/build/arm_neon.gypi:33: # Disable LTO due to compiler bug.
On 2015/06/18 09:06:23, kjellander (OOO til Aug 31) wrote:
> Please add a TODO + reference to the bug.
> Maybe even create another bug in the WebRTC tracker to cover re-enabling this?

Done.

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/common.gypi
File webrtc/build/common.gypi (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/common.gypi#...
webrtc/build/common.gypi:124: # Determines whether NEON code will be built. It
might not be selected at
On 2015/06/18 09:06:23, kjellander (OOO til Aug 31) wrote:
> I assume you mean "WebRTC doesn't support selecting it at runtime" ? Please
> phrase it that way since it's unclear to me in the current comment.

No, that's not what I meant :)

Just removed the run-time note to avoid confusion.

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/webrtc.gni
File webrtc/build/webrtc.gni (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/build/webrtc.gni#n...
webrtc/build/webrtc.gni:108: # Determines whether NEON code will be built. It
might not be selected at
On 2015/06/18 09:06:23, kjellander (OOO til Aug 31) wrote:
> Same comment update as in common.gypi...

Done.

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:603: # Disable LTO due to compiler bug.
On 2015/06/18 09:06:23, kjellander (OOO til Aug 31) wrote:
> Please link to the bug etc, similar to my previous comment.

Done.

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/BUILD.gn (right):

https://codereview.webrtc.org/1181373004/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/BUILD.gn:258: # Disable LTO due to compiler bug.
On 2015/06/18 09:06:23, kjellander (OOO til Aug 31) wrote:
> Add link to the bug as previous comment.

Done.

[Andrew MacDonald, 2015-06-25 01:09:15]

The CQ bit was checked by andrew@webrtc.org

[Andrew MacDonald, 2015-06-25 01:09:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jridges@masque.com,
kjellander@webrtc.org, zhongwei.yao@chromium.org
Link to the patchset: https://codereview.webrtc.org/1181373004/#ps80001 (title:
"Fix comments.")

[commit-bot: I haz the power, 2015-06-25 01:09:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181373004/80001

[commit-bot: I haz the power, 2015-06-25 01:13:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-25 01:13:27]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/32)

[Andrew MacDonald, 2015-06-25 01:26:05]

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