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

Configure f16 and f128 support for WebAssembly #665

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

kleisauke
Copy link
Contributor

@kleisauke kleisauke commented Aug 11, 2024

See: #652 (comment)

Tested with:

Details
$ curl https://sh.rustup.rs -sSf | sh -s -- -y --profile minimal --target wasm32-unknown-emscripten --default-toolchain nightly-2024-08-11 --component rust-src
$ source "$HOME/.cargo/env"
$ cargo new foo
$ cd foo
$ RUSTFLAGS="-Clink-arg=-Wl,--fatal-warnings" cargo build --release -Zbuild-std --target wasm32-unknown-emscripten
[...]
wasm-ld: error: function signature mismatch: __truncsfhf2
>>> defined as (f32) -> i32 in /home/kleisauke/foo/target/wasm32-unknown-emscripten/release/deps/libcompiler_builtins-903959ee8c3bdcea.rlib(compiler_builtins-903959ee8c3bdcea.compiler_builtins.f8125c6c5a7ecb3b-cgu.08.rcgu.o)
>>> defined as (f32) -> f32 in /home/kleisauke/foo/target/wasm32-unknown-emscripten/release/deps/libcompiler_builtins-903959ee8c3bdcea.rlib(compiler_builtins-903959ee8c3bdcea.compiler_builtins.f8125c6c5a7ecb3b-cgu.04.rcgu.o)

wasm-ld: error: function signature mismatch: __extendhfsf2
>>> defined as (i32) -> f32 in /home/kleisauke/foo/target/wasm32-unknown-emscripten/release/deps/libcompiler_builtins-903959ee8c3bdcea.rlib(compiler_builtins-903959ee8c3bdcea.compiler_builtins.f8125c6c5a7ecb3b-cgu.03.rcgu.o)
>>> defined as (f32) -> f32 in /home/kleisauke/foo/target/wasm32-unknown-emscripten/release/deps/libcompiler_builtins-903959ee8c3bdcea.rlib(compiler_builtins-903959ee8c3bdcea.compiler_builtins.f8125c6c5a7ecb3b-cgu.08.rcgu.o)
[...]
error: could not compile `foo` (bin "foo") due to 1 previous error
$ cat >> $(rustc --print sysroot)/lib/rustlib/src/rust/library/Cargo.toml <<EOL
compiler_builtins = { git = "https://github.com/kleisauke/compiler-builtins.git", branch = "wasm-f16-f128-feature" }
EOL
$ RUSTFLAGS="-Clink-arg=-Wl,--fatal-warnings" cargo build --release -Zbuild-std --target wasm32-unknown-emscripten
[...]
    Finished `release` profile [optimized] target(s) in 7.85s

build.rs Outdated Show resolved Hide resolved
@kleisauke kleisauke force-pushed the wasm-f16-f128-feature branch from b4fa4c3 to 6a80b90 Compare August 11, 2024 15:26
@kleisauke kleisauke changed the title Configure f16 and f128 support for wasm32/wasm64 Configure f16 and f128 support for WebAssembly Aug 11, 2024
@kleisauke
Copy link
Contributor Author

I've updated the reproducer in the initial post. It seems that the function signature mismatch issues are treated as warnings by default, but they become visible when passing RUSTFLAGS="-Clink-arg=-Wl,--fatal-warnings". This behavior is automatically enforced when using Emscripten's STRICT mode.
https://github.com/emscripten-core/emscripten/blob/c614fa57ea8048a34341891dbd6f99fb78b6869a/tools/building.py#L267-L268

CI is still failing for wasm32-unknown-unknown. I think we need to conditionally disable this test somehow:

#[cfg(not(feature = "no-f16-f128"))]
impl_testio!(float f16, f128);

@tgross35
Copy link
Contributor

I've updated the reproducer in the initial post. It seems that the function signature mismatch issues are treated as warnings by default, but they become visible when passing RUSTFLAGS="-Clink-arg=-Wl,--fatal-warnings". This behavior is automatically enforced when using Emscripten's STRICT mode. https://github.com/emscripten-core/emscripten/blob/c614fa57ea8048a34341891dbd6f99fb78b6869a/tools/building.py#L267-L268

Just to see, I tested making linker errors fatal on all targets in #667. wasm32-unknown-unknown seems to fail for other reasons.

CI is still failing for wasm32-unknown-unknown. I think we need to conditionally disable this test somehow:

#[cfg(not(feature = "no-f16-f128"))]
impl_testio!(float f16, f128);

Ah, these will be the only targets that are disabled in /build.rs but tested in CI. I think we need to emit the same enable_f16 and enable_f128 feature in testcrate, then gate relevant things. To avoid duplicating the logic you should be able to do something like:

// testcrate/build.rs

mod builtins_build {
    include!("../build.rs");
}

fn main() {
    // ...

    // will need to mark some things public in `/build.rs`
    builtins_build::configure_f16_f128(builtins_build::Target::from_env());
}

I would only do the enable_f16 gating for now since enable_f128 isn't relevant for testcrate at this time.

@kleisauke
Copy link
Contributor Author

Thanks! I just pushed commit 42ac64e. However, CI still fails for the wasm32-unknown-unknown target, curiously.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Thanks! I just pushed commit 42ac64e. However, CI still fails for the wasm32-unknown-unknown target, curiously.

Unfortunately the files in benches/ also need to be updated, so anything that tests f16 should get gated behind f16_enabled - both the float_bench! and the criterion_group! invocations will need adjusting. The testcrate/src/bench.rs change is needed too.

Some of the benches already have different criterion_group! per cfg, e.g.

#[cfg(not(any(target_arch = "powerpc", target_arch = "powerpc64")))]
criterion_group!(
float_extend,
extend_f16_f32,
extend_f16_f128,
extend_f32_f64,
extend_f32_f128,
extend_f64_f128,
);
// FIXME(#655): `f16` tests disabled until we can bootstrap symbols
#[cfg(any(target_arch = "powerpc", target_arch = "powerpc64"))]
criterion_group!(
float_extend,
extend_f32_f64,
extend_f32_f128,
extend_f64_f128,
);
. When updating, it is probably better to not use criterion_group and just configure manually, something like

pub fn float_extend() {
    let mut criterion = Criterion::default().configure_from_args();

    #[cfg(f16_enabled)]
    {
        ...
    }
    
    extend_f32_f64(&mut criterion);
    extend_f32_f64(&mut criterion);
}

testcrate/build.rs Outdated Show resolved Hide resolved
configure.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
configure.rs Show resolved Hide resolved
testcrate/src/bench.rs Outdated Show resolved Hide resolved
@kleisauke kleisauke force-pushed the wasm-f16-f128-feature branch from 6fb223f to 1cafa03 Compare August 12, 2024 11:18
@kleisauke
Copy link
Contributor Author

kleisauke commented Aug 12, 2024

All done! This is now ready for review. Thanks to @beetrees and @tgross35 for the help. :)

@kleisauke kleisauke marked this pull request as ready for review August 12, 2024 11:26
@kleisauke kleisauke force-pushed the wasm-f16-f128-feature branch from 1cafa03 to 35c5554 Compare August 12, 2024 12:57
@tgross35 tgross35 merged commit 7f9209d into rust-lang:master Aug 12, 2024
24 checks passed
@tgross35
Copy link
Contributor

Thanks! You can submit a PR to update rust-lang/rust to 0.1.119 once #668 merges.

@tgross35
Copy link
Contributor

Ah, publish failed because build.rs uses multiple files. @kleisauke can you submit a PR to either add the new file to the include list

'/build.rs',
, or have testcrate/build.rs just include the entire /build.rs as mentioned here #665 (comment)?

@kleisauke
Copy link
Contributor Author

kleisauke commented Aug 12, 2024

Apologies for the breakage. I'll submit a PR to add the new file to the include list.

Initially, I attempted to include the entire /build.rs into testcrate/build.rs, but that approach failed, as shown here:
https://github.com/kleisauke/compiler-builtins/actions/runs/10356628453/job/28666867698#step:13:519

This is why I decided to move that logic into a separate file.

@tgross35
Copy link
Contributor

No worries, thanks for the clarification. I'll take a look when it is up.

@kleisauke
Copy link
Contributor Author

I just opened PR #669 for this.

@kleisauke kleisauke deleted the wasm-f16-f128-feature branch August 12, 2024 17:47
@tgross35
Copy link
Contributor

0.1.119 published with that fix, feel free to submit a rust-lang/rust PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants