Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable f16 in assembly on aarch64 platforms that support it #127043

Closed
wants to merge 1 commit into from

Conversation

lengrongfu
Copy link
Contributor

@lengrongfu lengrongfu commented Jun 27, 2024

Issue: #125398

About pr: #126070

Signed-off-by: rongfu.leng <lenronfu@gmail.com>
@rustbot
Copy link
Collaborator

rustbot commented Jun 27, 2024

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 27, 2024
@lengrongfu
Copy link
Contributor Author

r? @tgross35

@rustbot
Copy link
Collaborator

rustbot commented Jun 27, 2024

Failed to set assignee to tgross35: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@beetrees
Copy link
Contributor

r? @Amanieu since you reviewed the other f16 inline ASM PRs.

@rustbot rustbot assigned Amanieu and unassigned lcnr Jun 27, 2024
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 46)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 49)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 61)
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
failures:

---- [assembly] tests/assembly/asm/aarch64-types.rs#arm64ec stdout ----

error in revision `arm64ec`: compilation failed!
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/assembly/asm/aarch64-types.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--cfg" "arm64ec" "--check-cfg" "cfg(FALSE,aarch64,arm64ec)" "-O" "-Cdebug-assertions=no" "--emit" "asm" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/assembly/asm/aarch64-types.arm64ec/aarch64-types.s" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/assembly/asm/aarch64-types.arm64ec/auxiliary" "--target" "arm64ec-pc-windows-msvc"
--- stderr -------------------------------
error: unexpected end of macro invocation
##[error]   --> /checkout/tests/assembly/asm/aarch64-types.rs:179:29
    |
    |
119 | macro_rules! check {
    | ------------------ when calling this macro
...
179 | check!(reg_f16 f16 reg "mov");
    |                             ^ missing tokens in macro arguments
    |
note: while trying to match meta-variable `$modifier:literal`
    |
    |
120 |     ($func:ident $ty:ident $class:ident $mov:literal $modifier:literal) => {

warning: unexpected `cfg` condition name: `neon`
##[warning]   --> /checkout/tests/assembly/asm/aarch64-types.rs:275:7
    |
    |
275 | #[cfg(neon)]
    |       ^^^^ help: found config with similar value: `target_feature = "neon"`
    |
    = help: expected names are: `FALSE`, `aarch64`, `arm64ec`, `clippy`, `debug_assertions`, `doc`, `doctest`, `miri`, `overflow_checks`, `panic`, `proc_macro`, `relocation_model`, `rustfmt`, `sanitize`, `sanitizer_cfi_generalize_pointers`, `sanitizer_cfi_normalize_integers`, `target_abi`, `target_arch`, `target_endian`, `target_env`, `target_family`, `target_feature`, `target_has_atomic`, `target_has_atomic_equal_alignment`, `target_has_atomic_load_store`, `target_os`, `target_pointer_width`, `target_thread_local`, `target_vendor`, `test`, `ub_checks`, `unix`, and `windows`
    = help: to expect this configuration use `--check-cfg=cfg(neon)`
    = note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg.html> for more information about checking conditional configuration

warning: unexpected `cfg` condition name: `neon`
##[warning]   --> /checkout/tests/assembly/asm/aarch64-types.rs:282:7
    |
    |
282 | #[cfg(neon)]
    |       ^^^^ help: found config with similar value: `target_feature = "neon"`
    |
    = help: to expect this configuration use `--check-cfg=cfg(neon)`
    = note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg.html> for more information about checking conditional configuration
error: aborting due to 1 previous error; 2 warnings emitted
------------------------------------------



---- [assembly] tests/assembly/asm/aarch64-types.rs#aarch64 stdout ----

error in revision `aarch64`: compilation failed!
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/assembly/asm/aarch64-types.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--cfg" "aarch64" "--check-cfg" "cfg(FALSE,aarch64,arm64ec)" "-O" "-Cdebug-assertions=no" "--emit" "asm" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/assembly/asm/aarch64-types.aarch64/aarch64-types.s" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/assembly/asm/aarch64-types.aarch64/auxiliary" "--target" "aarch64-unknown-linux-gnu"
--- stderr -------------------------------
error: unexpected end of macro invocation
##[error]   --> /checkout/tests/assembly/asm/aarch64-types.rs:179:29
    |
    |
119 | macro_rules! check {
    | ------------------ when calling this macro
...
179 | check!(reg_f16 f16 reg "mov");
    |                             ^ missing tokens in macro arguments
    |
note: while trying to match meta-variable `$modifier:literal`
    |
    |
120 |     ($func:ident $ty:ident $class:ident $mov:literal $modifier:literal) => {

warning: unexpected `cfg` condition name: `neon`
##[warning]   --> /checkout/tests/assembly/asm/aarch64-types.rs:275:7
    |
    |
275 | #[cfg(neon)]
    |       ^^^^ help: found config with similar value: `target_feature = "neon"`
    |
    = help: expected names are: `FALSE`, `aarch64`, `arm64ec`, `clippy`, `debug_assertions`, `doc`, `doctest`, `miri`, `overflow_checks`, `panic`, `proc_macro`, `relocation_model`, `rustfmt`, `sanitize`, `sanitizer_cfi_generalize_pointers`, `sanitizer_cfi_normalize_integers`, `target_abi`, `target_arch`, `target_endian`, `target_env`, `target_family`, `target_feature`, `target_has_atomic`, `target_has_atomic_equal_alignment`, `target_has_atomic_load_store`, `target_os`, `target_pointer_width`, `target_thread_local`, `target_vendor`, `test`, `ub_checks`, `unix`, and `windows`
    = help: to expect this configuration use `--check-cfg=cfg(neon)`
    = note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg.html> for more information about checking conditional configuration

warning: unexpected `cfg` condition name: `neon`
##[warning]   --> /checkout/tests/assembly/asm/aarch64-types.rs:282:7
    |
    |
282 | #[cfg(neon)]
    |       ^^^^ help: found config with similar value: `target_feature = "neon"`
    |
    = help: to expect this configuration use `--check-cfg=cfg(neon)`
    = note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg.html> for more information about checking conditional configuration
error: aborting due to 1 previous error; 2 warnings emitted
------------------------------------------


Comment on lines +271 to +283
// neon-LABEL: vreg_f16x4:
// neon: @APP
// neon: vmov.f64 d{{[0-9]+}}, d{{[0-9]+}}
// neon: @NO_APP
#[cfg(neon)]
check!(vreg_f16x4 f16x4 vreg "vmov.f64");

// neon-LABEL: vreg_f16x8:
// neon: @APP
// neon: vorr q{{[0-9]+}}, q{{[0-9]+}}, q{{[0-9]+}}
// neon: @NO_APP
#[cfg(neon)]
check!(vreg_f16x8 f16x8 vreg "vmov");
Copy link
Contributor

@tgross35 tgross35 Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these neon labels are actually getting tested, you need to add neon in the //@ revisions: list at the top and make sure --cfg neon gets passed for at least one of the tests (like

//@ revisions: base d32 neon
//@ assembly-output: emit-asm
//@ compile-flags: --target armv7-unknown-linux-gnueabihf
//@ compile-flags: -C opt-level=0
//@[d32] compile-flags: -C target-feature=+d32
//@[neon] compile-flags: -C target-feature=+neon --cfg d32
//@[neon] filecheck-flags: --check-prefix d32
)

I'm not sure what combinations it makes sense to test, there's also a fp16 target feature. Cc @workingjubilee since you did some work on neon and flags

Copy link
Contributor

@beetrees beetrees Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The neon target feature is enabled by default on aarch64-unknown-linux-gnu and arm64ec-pc-windows-msvc, so there's no need to have any cfg unless it's been explicitly disabled.

However, once the cfg is removed there might be compilation failures on arm64ec-pc-windows-msvc due to llvm/llvm-project#94434, in which case the options are to either change the check! macro to not pass/return f16 by value or just cfg(aarch64) the f16 test cases and leave a FIXME(f16_f128) comment.

@tgross35
Copy link
Contributor

@rustbot label +F-f16_and_f128

@rustbot rustbot added the F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` label Jun 27, 2024
@Amanieu
Copy link
Member

Amanieu commented Jul 28, 2024

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 28, 2024
@tgross35
Copy link
Contributor

tgross35 commented Aug 21, 2024

@lengrongfu are you still interested in working on this? No problem if not, somebody else may be able to pick up your work and get it over the line.

@lengrongfu
Copy link
Contributor Author

Sorry, I don't have enough time to fix this.

@lengrongfu lengrongfu closed this Aug 21, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2024
…, r=<try>

Add `f16` and `f128` inline ASM support for `aarch64`

Adds `f16` and `f128` inline ASM support for `aarch64`. SIMD vector types are taken from [the ARM intrinsics list](https://developer.arm.com/architectures/instruction-sets/intrinsics/#f:`@navigationhierarchiesreturnbasetype=[float]&f:@navigationhierarchieselementbitsize=[16]&f:@navigationhierarchiesarchitectures=[A64]).` Based on the work of `@lengrongfu` in rust-lang#127043.

Relevant issue: rust-lang#125398
Tracking issue: rust-lang#116909

`@rustbot` label +F-f16_and_f128

try-job: aarch64-gnu
try-job: aarch64-apple
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2024
…, r=<try>

Add `f16` and `f128` inline ASM support for `aarch64`

Adds `f16` and `f128` inline ASM support for `aarch64`. SIMD vector types are taken from [the ARM intrinsics list](https://developer.arm.com/architectures/instruction-sets/intrinsics/#f:`@navigationhierarchiesreturnbasetype=[float]&f:@navigationhierarchieselementbitsize=[16]&f:@navigationhierarchiesarchitectures=[A64]).` Based on the work of `@lengrongfu` in rust-lang#127043.

Relevant issue: rust-lang#125398
Tracking issue: rust-lang#116909

`@rustbot` label +F-f16_and_f128

try-job: aarch64-gnu
try-job: aarch64-apple
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 27, 2024
…64, r=Amanieu

Add `f16` and `f128` inline ASM support for `aarch64`

Adds `f16` and `f128` inline ASM support for `aarch64`. SIMD vector types are taken from [the ARM intrinsics list](https://developer.arm.com/architectures/instruction-sets/intrinsics/#f:`@navigationhierarchiesreturnbasetype=[float]&f:@navigationhierarchieselementbitsize=[16]&f:@navigationhierarchiesarchitectures=[A64]).` Based on the work of `@lengrongfu` in rust-lang#127043.

Relevant issue: rust-lang#125398
Tracking issue: rust-lang#116909

`@rustbot` label +F-f16_and_f128

try-job: aarch64-gnu
try-job: aarch64-apple
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2024
Rollup merge of rust-lang#129536 - beetrees:f16-f128-inline-asm-aarch64, r=Amanieu

Add `f16` and `f128` inline ASM support for `aarch64`

Adds `f16` and `f128` inline ASM support for `aarch64`. SIMD vector types are taken from [the ARM intrinsics list](https://developer.arm.com/architectures/instruction-sets/intrinsics/#f:`@navigationhierarchiesreturnbasetype=[float]&f:@navigationhierarchieselementbitsize=[16]&f:@navigationhierarchiesarchitectures=[A64]).` Based on the work of `@lengrongfu` in rust-lang#127043.

Relevant issue: rust-lang#125398
Tracking issue: rust-lang#116909

`@rustbot` label +F-f16_and_f128

try-job: aarch64-gnu
try-job: aarch64-apple
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants