[Opus] Only include NEON sources and macros when arm_use_neon==true.

[Opus] Only include NEON sources and macros when arm_use_neon==true.

arm_neon.h throws an error when included in a non-neon build. Only
include neon-related sources when use_arm_neon is true.

BUG=657874
Committed: https://crrev.com/22d06b0c916aa422a57e58d634cb73c2909b320d
Cr-Commit-Position: refs/heads/master@{#435027}

[slan, 2016-10-19 21:30:21]

slan@chromium.org changed reviewers:
+ henrika@chromium.org, wzhong@chromium.org

[slan, 2016-10-19 21:30:21]

Follow-up to https://codereview.chromium.org/2411373002, which breaks non-neon
arm builds.

PTAL!

[slan, 2016-10-19 21:36:32]

The CQ bit was checked by slan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 21:37:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wzhong, 2016-10-19 22:36:22]

lgtm

[commit-bot: I haz the power, 2016-10-20 00:33:11]

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

[commit-bot: I haz the power, 2016-10-20 00:33:12]

Dry run: This issue passed the CQ dry run.

[henrika (OOO until Aug 14), 2016-10-20 07:37:13]

Description was changed from

==========
[Opus] Only include NEON sources and macros when arm_use_neon==true.

arm_neon.h throws an error when included in a non-neon build. Only
include neon-related sources when use_arm_neon is true.

BUG=
==========

to

==========
[Opus] Only include NEON sources and macros when arm_use_neon==true.

arm_neon.h throws an error when included in a non-neon build. Only
include neon-related sources when use_arm_neon is true.

BUG=
==========

[henrika (OOO until Aug 14), 2016-10-20 07:37:13]

henrika@chromium.org changed reviewers:
+ minyue@chromium.org

[henrika (OOO until Aug 14), 2016-10-20 07:37:43]

minyue@: PTAL on my behalf.

[minyue, 2016-10-20 07:55:24]

https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn
File third_party/opus/BUILD.gn (right):

https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn#n...
third_party/opus/BUILD.gn:369: if (use_opus_rtcd) {
If I am not mistaken, RTCD can determine neon at runtime, and
OPUS_ARM_MAY_HAVE_NEON is supposed to work with all compilers.

Would you add a bug issue (and update the BUG field in the CL's description)?
There you can write the way to reproduce the build error.

[wzhong, 2016-10-20 15:30:11]

https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn
File third_party/opus/BUILD.gn (right):

https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn#n...
third_party/opus/BUILD.gn:369: if (use_opus_rtcd) {
On 2016/10/20 07:55:24, minyue wrote:
> If I am not mistaken, RTCD can determine neon at runtime, and
> OPUS_ARM_MAY_HAVE_NEON is supposed to work with all compilers.
> 
> Would you add a bug issue (and update the BUG field in the CL's description)?
> There you can write the way to reproduce the build error.

Yeah. A bug would help.

The problem is the source file includes "arm_neon.h", which requres =mfpu=enon
for compiling.

Otherwise you'd get "You must enable NEON instructions (e.g. -mfloat-abi=softfp
-mfpu=neon) to use arm_neon.h".

https://cs.corp.google.com/eureka_internal/prebuilt/toolchain/armv7a/usr/lib/...

[slan, 2016-10-20 16:27:07]

Description was changed from

==========
[Opus] Only include NEON sources and macros when arm_use_neon==true.

arm_neon.h throws an error when included in a non-neon build. Only
include neon-related sources when use_arm_neon is true.

BUG=
==========

to

==========
[Opus] Only include NEON sources and macros when arm_use_neon==true.

arm_neon.h throws an error when included in a non-neon build. Only
include neon-related sources when use_arm_neon is true.

BUG=657874
==========

[slan, 2016-10-20 16:33:11]

https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn
File third_party/opus/BUILD.gn (right):

https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn#n...
third_party/opus/BUILD.gn:369: if (use_opus_rtcd) {
On 2016/10/20 07:55:24, minyue wrote:
> If I am not mistaken, RTCD can determine neon at runtime, and
> OPUS_ARM_MAY_HAVE_NEON is supposed to work with all compilers.
> 
> Would you add a bug issue (and update the BUG field in the CL's description)?
> There you can write the way to reproduce the build error.

Tagged the bug.

To your point, OPUS_ARM_MAY_HAVE_NEON sounds like it should work unconditionally
at runtime, with neon or not. However, there are lots of functions whose
callsites are guarded by these macros, with definitions in the excluded files.
So if we don't put these macros in the conditional, we have a bunch of missing
symbols.

[minyue, 2016-10-20 18:02:56]

minyue@chromium.org changed reviewers:
+ flim@chromium.org

[minyue, 2016-10-20 18:03:13]

On 2016/10/20 16:33:11, slan wrote:
> https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn
> File third_party/opus/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn#n...
> third_party/opus/BUILD.gn:369: if (use_opus_rtcd) {
> On 2016/10/20 07:55:24, minyue wrote:
> > If I am not mistaken, RTCD can determine neon at runtime, and
> > OPUS_ARM_MAY_HAVE_NEON is supposed to work with all compilers.
> > 
> > Would you add a bug issue (and update the BUG field in the CL's
description)?
> > There you can write the way to reproduce the build error.
> 
> Tagged the bug.
> 
> To your point, OPUS_ARM_MAY_HAVE_NEON sounds like it should work
unconditionally
> at runtime, with neon or not. However, there are lots of functions whose
> callsites are guarded by these macros, with definitions in the excluded files.
> So if we don't put these macros in the conditional, we have a bunch of missing
> symbols.

＋flim to take a look too.

[minyue, 2016-10-20 18:04:33]

On 2016/10/20 16:33:11, slan wrote:
> https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn
> File third_party/opus/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn#n...
> third_party/opus/BUILD.gn:369: if (use_opus_rtcd) {
> On 2016/10/20 07:55:24, minyue wrote:
> > If I am not mistaken, RTCD can determine neon at runtime, and
> > OPUS_ARM_MAY_HAVE_NEON is supposed to work with all compilers.
> > 
> > Would you add a bug issue (and update the BUG field in the CL's
description)?
> > There you can write the way to reproduce the build error.
> 
> Tagged the bug.
> 
> To your point, OPUS_ARM_MAY_HAVE_NEON sounds like it should work
unconditionally
> at runtime, with neon or not. However, there are lots of functions whose
> callsites are guarded by these macros, with definitions in the excluded files.
> So if we don't put these macros in the conditional, we have a bunch of missing
> symbols.

so, is it possible to add missing files?

[flim-chromium, 2016-10-20 18:40:31]

lgtm

[slan, 2016-10-20 19:14:25]

On 2016/10/20 18:04:33, minyue wrote:
> On 2016/10/20 16:33:11, slan wrote:
> > https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn
> > File third_party/opus/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn#n...
> > third_party/opus/BUILD.gn:369: if (use_opus_rtcd) {
> > On 2016/10/20 07:55:24, minyue wrote:
> > > If I am not mistaken, RTCD can determine neon at runtime, and
> > > OPUS_ARM_MAY_HAVE_NEON is supposed to work with all compilers.
> > > 
> > > Would you add a bug issue (and update the BUG field in the CL's
> description)?
> > > There you can write the way to reproduce the build error.
> > 
> > Tagged the bug.
> > 
> > To your point, OPUS_ARM_MAY_HAVE_NEON sounds like it should work
> unconditionally
> > at runtime, with neon or not. However, there are lots of functions whose
> > callsites are guarded by these macros, with definitions in the excluded
files.
> > So if we don't put these macros in the conditional, we have a bunch of
missing
> > symbols.
> 
> so, is it possible to add missing files?

The files I was referring to are the ones I've moved inside the conditional. I
was justifying why the files and the macros must be coupled.

[slan, 2016-10-21 18:01:39]

On 2016/10/20 19:14:25, slan wrote:
> On 2016/10/20 18:04:33, minyue wrote:
> > On 2016/10/20 16:33:11, slan wrote:
> > >
https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn
> > > File third_party/opus/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2427333004/diff/1/third_party/opus/BUILD.gn#n...
> > > third_party/opus/BUILD.gn:369: if (use_opus_rtcd) {
> > > On 2016/10/20 07:55:24, minyue wrote:
> > > > If I am not mistaken, RTCD can determine neon at runtime, and
> > > > OPUS_ARM_MAY_HAVE_NEON is supposed to work with all compilers.
> > > > 
> > > > Would you add a bug issue (and update the BUG field in the CL's
> > description)?
> > > > There you can write the way to reproduce the build error.
> > > 
> > > Tagged the bug.
> > > 
> > > To your point, OPUS_ARM_MAY_HAVE_NEON sounds like it should work
> > unconditionally
> > > at runtime, with neon or not. However, there are lots of functions whose
> > > callsites are guarded by these macros, with definitions in the excluded
> files.
> > > So if we don't put these macros in the conditional, we have a bunch of
> missing
> > > symbols.
> > 
> > so, is it possible to add missing files?
> 
> The files I was referring to are the ones I've moved inside the conditional. I
> was justifying why the files and the macros must be coupled.

I think I need an L-G-T-M from an OWNER in opus/ right? henrika@?

[henrika (OOO until Aug 14), 2016-10-21 18:05:17]

Yes, but I rely on OK from minyue@ who knows the code base better than I do.

If you interpret feedback from minyue@ as OK then LGTM from my side as well.

[slan, 2016-10-21 18:07:11]

On 2016/10/21 18:05:17, henrika wrote:
> Yes, but I rely on OK from minyue@ who knows the code base better than I do.
> 
> If you interpret feedback from minyue@ as OK then LGTM from my side as well.

minyue@: Do you have further thoughts?

[minyue, 2016-10-21 20:44:20]

lgtm

[slan, 2016-11-29 16:58:53]

The CQ bit was checked by slan@chromium.org

[commit-bot: I haz the power, 2016-11-29 16:59:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 17:51:51]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1480438733763770, "parent_rev":
"08219c4a7d207ed1a4366b93f5b9bcc47eb363ac", "commit_rev":
"eac2cf6536d923616784629e6c177a7a64fbc38d"}

[commit-bot: I haz the power, 2016-11-29 17:52:19]

Description was changed from

==========
[Opus] Only include NEON sources and macros when arm_use_neon==true.

arm_neon.h throws an error when included in a non-neon build. Only
include neon-related sources when use_arm_neon is true.

BUG=657874
==========

to

==========
[Opus] Only include NEON sources and macros when arm_use_neon==true.

arm_neon.h throws an error when included in a non-neon build. Only
include neon-related sources when use_arm_neon is true.

BUG=657874
==========

[commit-bot: I haz the power, 2016-11-29 17:52:20]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-29 17:54:43]

Description was changed from

==========
[Opus] Only include NEON sources and macros when arm_use_neon==true.

arm_neon.h throws an error when included in a non-neon build. Only
include neon-related sources when use_arm_neon is true.

BUG=657874
==========

to

==========
[Opus] Only include NEON sources and macros when arm_use_neon==true.

arm_neon.h throws an error when included in a non-neon build. Only
include neon-related sources when use_arm_neon is true.

BUG=657874

Committed: https://crrev.com/22d06b0c916aa422a57e58d634cb73c2909b320d
Cr-Commit-Position: refs/heads/master@{#435027}
==========

[commit-bot: I haz the power, 2016-11-29 17:54:44]

Patchset 1 (id:??) landed as
https://crrev.com/22d06b0c916aa422a57e58d634cb73c2909b320d
Cr-Commit-Position: refs/heads/master@{#435027}