Closed Bug 1397807 Opened 7 years ago Closed 6 years ago

Set --enable-rust-simd on android/arm

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Platform:
ARM
Android
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: m_kato, Assigned: hsivonen)

References

Details

Attachments

(1 file)

Bug 1397807 - Allow rustc to emit NEON instructions when clang does on ARMv7 and use NEON in encoding_rs.
59 bytes, text/x-review-board-request
glandium
: review+
Details 
By bug 1261841, aarch64 etc uses --enable-rust-simd.  But arm doesn't set it.

By bug 1345267, Android/arm requires neon support, so we should set --enable-rust-simd on android/arm.
Product: Core → Firefox Build System
Depends on: 1466807
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Any ideas why the config/rules.mk change doesn't take effect? (The symptoms of the logged build error are those of trying to build the simd crate without -C target_feature=+neon in RUSTFLAGS. We'll need the flag until we switch to a fully NEON-enabled Rust target. See https://github.com/rust-lang/rust/pull/49902 )
Flags: needinfo?(mh+mozilla)
Because you end up with a command like:
RUSTFLAGS='...' -C target_feature+=neon cargo ...

Relatedly, you should put this behind BUILD_ARM_NEON too, because CPU_ARCH=arm is too broad.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #4)
> Relatedly, you should put this behind BUILD_ARM_NEON too, because
> CPU_ARCH=arm is too broad.

We already require distro builds for non-SSE2 x86 to omit --enable-rust-simd. Can we not similarly require non-NEON ARM builds to omit --enable-rust-simd?
Flags: needinfo?(mh+mozilla)
(In reply to Henri Sivonen (:hsivonen) from comment #5)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > Relatedly, you should put this behind BUILD_ARM_NEON too, because
> > CPU_ARCH=arm is too broad.
> 
> We already require distro builds for non-SSE2 x86 to omit
> --enable-rust-simd. Can we not similarly require non-NEON ARM builds to omit
> --enable-rust-simd?

I'm pretty sure this requirement has the opposite effect of what you'd want. As evidence: *I* forgot to use --enable-rust-simd, and I'm in the loop! And there's no reason it shouldn't be the default on platforms where simd is always available on the platform (e.g. x86-64 always has sse2, arm64 always has neon), or on platforms where we're already building gecko with simd (e.g. BUILD_ARM_NEON on arm).
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #7)
> And
> there's no reason it shouldn't be the default on platforms where simd is
> always available on the platform

For release builds, I agree. For developer builds, there is a reason: --enable-rust-simd unlocks nightly-Rust-only features on release-channel Rust, and we don't want accidental dependencies on nightly-Rust-only features to get in the tree. (SIMD isn't accidental.)

What's the correct way to make --enable-rust-simd the default for distro release builds but not for local development builds? If BUILD_ARM_NEON should control the ARMv7 NEON availability, what should distinguish x86+SSE2 from x86-without-SSE2?
Flags: needinfo?(mh+mozilla)
Comment on attachment 8983372 [details]
Bug 1397807 - Allow rustc to emit NEON instructions when clang does on ARMv7 and use NEON in encoding_rs.

https://reviewboard.mozilla.org/r/249282/#review256754

::: config/rules.mk:855
(Diff revision 3)
> -rustflags_override = RUSTFLAGS='$(MOZ_RUST_DEFAULT_FLAGS) $(RUSTFLAGS)'
> +rustflags_neon =
> +rust_unlock_unstable =
> +ifdef MOZ_RUST_SIMD
> +rust_unlock_unstable += RUSTC_BOOTSTRAP=1
> +ifeq (arm,$(CPU_ARCH))
> +rustflags_neon += '-C target_feature=+neon'

I can live with the fact that for now, RUSTC_BOOTSTRAP=1 is set everywhere because it doesn't really have an effect (something that builds without will build the same with), but setting -C target_feature=+neon everywhere changes how every piece of rust is compiled, and I'm not really comfortable with this.

Especially, since the NDK docs still say that not all Android devices support NEON.

So, in fact, unless encoding_rs has a runtime fallback when neon is not supported, and that fallback is not built with autovectorization with neon, this can't be enabled on arm. aarch64 is another story, although the arm docs *do* say that there are specialized armv8 without neon too, but those are presumably more embedded things (OTOH, I wouldn't be entirely surprised if cheap chinese devices end up without neon).
Flags: needinfo?(mh+mozilla)
> there are specialized armv8 without neon too

there *can be* specialized armv8 without neon
(In reply to Mike Hommey [:glandium] from comment #9)
> Especially, since the NDK docs still say that not all Android devices
> support NEON.

We no longer support Android devices that don't have NEON.

> So, in fact, unless encoding_rs has a runtime fallback when neon is not supported,
> and that fallback is not built with autovectorization with neon, this can't be
> enabled on arm.

It can for Android, since we don't support non-NEON ARM configurations on Android. I agree that non-NEON GNU/Linux configuration needs to be available.
Comment on attachment 8983372 [details]
Bug 1397807 - Allow rustc to emit NEON instructions when clang does on ARMv7 and use NEON in encoding_rs.

https://reviewboard.mozilla.org/r/249282/#review256754

> I can live with the fact that for now, RUSTC_BOOTSTRAP=1 is set everywhere because it doesn't really have an effect (something that builds without will build the same with), but setting -C target_feature=+neon everywhere changes how every piece of rust is compiled, and I'm not really comfortable with this.
> 
> Especially, since the NDK docs still say that not all Android devices support NEON.
> 
> So, in fact, unless encoding_rs has a runtime fallback when neon is not supported, and that fallback is not built with autovectorization with neon, this can't be enabled on arm. aarch64 is another story, although the arm docs *do* say that there are specialized armv8 without neon too, but those are presumably more embedded things (OTOH, I wouldn't be entirely surprised if cheap chinese devices end up without neon).

As noted, while the NDK docs say not all Android devices support NEON, we don't support Android ARM devices that don't support NEON.
Flags: needinfo?(mh+mozilla)
(ReviewBoard appears to have eaten a comment of mine.)

glandium, do you still think toolkit/moz.configure needs to be magic and make --enable-rust-simd a no-op if BUILD_ARM_NEON is not set? If so, how should BUILD_ARM_NEON be checked there?

> setting -C target_feature=+neon everywhere changes how every piece of rust is compiled, and I'm not really comfortable with this.

Why is this a problem on Android considering that we don't support non-NEON ARM on Android anymore?
Blocks: 1402247
(In reply to Henri Sivonen (:hsivonen) from comment #14)
> (ReviewBoard appears to have eaten a comment of mine.)
> 
> glandium, do you still think toolkit/moz.configure needs to be magic and
> make --enable-rust-simd a no-op if BUILD_ARM_NEON is not set? If so, how
> should BUILD_ARM_NEON be checked there?

Let's ignore this part as long as it's not the default.
 
> > setting -C target_feature=+neon everywhere changes how every piece of rust is compiled, and I'm not really comfortable with this.
> 
> Why is this a problem on Android considering that we don't support non-NEON
> ARM on Android anymore?

It's not a problem per se, but it's a *much* larger change than "build encoding_rs with neon", since it impacts every single piece of rust code. Which also means it could very well uncover rust compiler bugs when it tries to do smart things with neon in other crates.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #15)
> > > setting -C target_feature=+neon everywhere changes how every piece of rust is compiled, and I'm not really comfortable with this.
> > 
> > Why is this a problem on Android considering that we don't support non-NEON
> > ARM on Android anymore?
> 
> It's not a problem per se, but it's a *much* larger change than "build
> encoding_rs with neon", since it impacts every single piece of rust code.

Yes.

> Which also means it could very well uncover rust compiler bugs when it tries
> to do smart things with neon in other crates.

Aren't we already compiling C++ code with clang with NEON enabled? That is, aren't we already exposing all our C++ to potential LLVM NEON autovectorizer bugs?

Apart from potential bugs, I'd expect us to want to enable the LLVM NEON autovectorizer for all code.
Flags: needinfo?(mh+mozilla)
> Aren't we already compiling C++ code with clang with NEON enabled?

As a matter of fact, we aren't. We're only enabling it on specific parts of the code.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #17)
> > Aren't we already compiling C++ code with clang with NEON enabled?
> 
> As a matter of fact, we aren't. We're only enabling it on specific parts of
> the code.

Why is that? Do we have a concrete reason to avoid the LLVM autovectorizer on ARMv7 or is this unspecific concern? I gather that before iOS went 64-bit-only, ARMv7 iPhones had NEON. Was the iOS ecosystem not exercising LLVM autovectorization to a degree that we should believe that it works? We aren't avoiding the LLVM autovectorizer on x86, x86_64 or aach64, right?
Flags: needinfo?(mh+mozilla)
Because nobody did the work to enable it when non-NEON was unsupported.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #19)
> Because nobody did the work to enable it when non-NEON was unsupported.

That doesn't look like a good reason to complicate the NEON enablement for Rust. Can we go ahead and land the patch here after the soft freeze ends on June 25th? (I don't know how to re-request review on review board after the review flag has been unset in the past.)

I filed bug 1469790 about compiling all C++ code with NEON enabled.
Flags: needinfo?(mh+mozilla)
I thought you were targetting 62. If you want this for the next cycle, then sure, but then, the approach is not the right one either. Building all rust code with neon shouldn't be tied to enabling simd for encoding_rs. If anything, if should be tied to C++ using neon.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #21)
> Building all rust
> code with neon shouldn't be tied to enabling simd for encoding_rs. If
> anything, if should be tied to C++ using neon.

I don't know how to do that. Can you provide more specific guidance on what the patch should do?
Flags: needinfo?(mh+mozilla)
I guess a reasonable thing to do is to use MOZ_FPU, but ideally, we'd need to resolve toolchain-default to what that actually means (although it very likely doesn't mean neon, so it's probably fine to ignore that for now). One complication is that MOZ_FPU is currently handled in old-configure... but that's not really a problem if your logic is in rules.mk.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #23)
> I guess a reasonable thing to do is to use MOZ_FPU, but ideally, we'd need
> to resolve toolchain-default to what that actually means (although it very
> likely doesn't mean neon, so it's probably fine to ignore that for now). One
> complication is that MOZ_FPU is currently handled in old-configure... but
> that's not really a problem if your logic is in rules.mk.

I'm still unsure what code I should be writing to pass review. In rules.mk, what MOZ_FPU check should I add relative to the currently-attached patch? I.e. what value should I be checking and what should the interaction with MOZ_RUST_SIMD be?
Flags: needinfo?(mh+mozilla)
ifeq (neon,$(MOZ_FPU))

You'll need to add an AC_SUBST(MOZ_FPU) in e.g. build/autoconf/arch.m4.
Flags: needinfo?(mh+mozilla)
The latest patch triggers -C target_feature=+neon on MOZ_FPU and not on --enable-rust-simd, but doesn't fix the issue from comment 7 that already applies to non-SSE2 x86.
Comment on attachment 8983372 [details]
Bug 1397807 - Allow rustc to emit NEON instructions when clang does on ARMv7 and use NEON in encoding_rs.

https://reviewboard.mozilla.org/r/249282/#review261526

::: config/rules.mk
(Diff revision 5)
> -rust_unlock_unstable =
> -ifdef MOZ_RUST_SIMD
> -rust_unlock_unstable += RUSTC_BOOTSTRAP=1
> -endif

doesn't seem necessary to move this.

::: toolkit/moz.configure:847
(Diff revision 5)
>  
>  @depends('--enable-rust-simd', target)
>  def rust_simd(value, target):
> -    # As of 2017-06-13, the simd crate only works on aarch64,
> -    # x86 and x86_64. It's meant to work on 32-bit ARM, too,
> -    # but currently does not.
> +    # As of 2018-06-05, the simd crate only works on aarch64,
> +    # armv7, x86 and x86_64.
> +    if target.cpu in ('aarch64', 'arm', 'x86', 'x86_64') and value:

I'd rather not mix changes here. That is, it would be better to enable neon in rust code first, and *then* use neon in encoding_rs.

Or at least not pretend in the commit message that only the latter is happening.
Attachment #8983372 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8983372 [details]
Bug 1397807 - Allow rustc to emit NEON instructions when clang does on ARMv7 and use NEON in encoding_rs.

https://reviewboard.mozilla.org/r/249282/#review261526

> doesn't seem necessary to move this.

Moved back.

> I'd rather not mix changes here. That is, it would be better to enable neon in rust code first, and *then* use neon in encoding_rs.
> 
> Or at least not pretend in the commit message that only the latter is happening.

I take the combination of r+ with this comment to mean that I should adjust the commit message, so I did that.
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a56101afb95
Allow rustc to emit NEON instructions when clang does on ARMv7 and use NEON in encoding_rs. r=glandium
https://hg.mozilla.org/mozilla-central/rev/9a56101afb95
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: