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 which platforms have f16 and f128 enabled by default #652

Merged
merged 8 commits into from
Aug 4, 2024

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Aug 2, 2024

By moving the logic for which platforms get symbols to compiler_builtins rather than rust-lang/rust, we can control where symbols get enabled without relying on Cargo features. Using Cargo features turned out to be a problem in 1.

This will help resolve errors like 2.

@tgross35 tgross35 force-pushed the control-f16-f128-feature branch 3 times, most recently from 9e31b97 to 773969c Compare August 2, 2024 20:13
@tgross35 tgross35 marked this pull request as draft August 2, 2024 20:15
@beetrees
Copy link
Contributor

beetrees commented Aug 2, 2024

For reference, I've locally tested compiling on every target Rust currently supports, and the only architectures/targets that I couldn't compile compiler-builtins with both f16 and f128 enabled are:

The only targets I wasn't able to compile (even with the no-f16-f128 feature enabled) were all tier 3:

I was able to build testcrate on all tier 1/2 targets that aren't listed as having issues above (I haven't checked tier 3 targets for testcrate), but I haven't run testcrate on all of them so it's possible CI could fail on a target not listed above.

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 2, 2024

I was going to ping you once I figured this out but you beat me to it. Thanks, that is incredibly helpful.

Do you think it is reasonable to configure disabling those targets here? As I mentioned in the diff I think it would be better in rust-lang/rust, but I can't come up with a good way to do it that doesn't rely on cargo features.

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 2, 2024

Are you testing on a version with LLVM19? I know llvm/llvm-project#93894 caused a problem when we were updating builtins but that has since been resolved. I am pleasantly surprised if that was the only selection failure for interfaces.

@beetrees
Copy link
Contributor

beetrees commented Aug 2, 2024

I think doing it here is probably the best solution at this point, given the issues with switching to the v2 resolver in rust-lang/rust#128359. Ultimately, as compiler-builtins is an internal crate for use in rust-lang/rust, I don't think it matters whether the workaround for LLVM bugs to make compiler-builtins compile on all targets is in rust-lang/rust or rust-lang/compiler-builtins: the end result is (apart from Cargo resolver issues) the same.

rustc --version -v:

rustc 1.82.0-nightly (28a58f2fa 2024-07-31)
binary: rustc
commit-hash: 28a58f2fa7f0c46b8fab8237c02471a915924fe5
commit-date: 2024-07-31
host: x86_64-unknown-linux-gnu
release: 1.82.0-nightly
LLVM version: 19.1.0

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 2, 2024

I don't think it matters whether the workaround for LLVM bugs to make compiler-builtins compile on all targets is in rust-lang/rust or rust-lang/compiler-builtins: the end result is (apart from Cargo resolver issues) the same.

Agreed. I am just not looking forward to the cycle of PR here -> release -> Rust PR -> Rust CI, if there are failures. But maybe this won't be much of an issue if there aren't too many completely broken targets.

I think the 7-31 nightly is the first one since rust-lang/rust#127513, so I guess that was all. Awesome.

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 2, 2024

Does this one actually crash or does it just have runtime issues?

@beetrees
Copy link
Contributor

beetrees commented Aug 2, 2024

It fails to compile with rustc-LLVM ERROR: SPARCv8 does not handle f128 in calls; pass indirectly; I'll post an update on the issue to make the current state of affairs clearer.

@tgross35 tgross35 changed the title Configure which platforms get f16 and f128 enabled by default Configure which platforms have f16 and f128` enabled by default Aug 2, 2024
@tgross35 tgross35 changed the title Configure which platforms have f16 and f128` enabled by default Configure which platforms have f16 and f128 enabled by default Aug 2, 2024
@tgross35 tgross35 mentioned this pull request Aug 2, 2024
@tgross35 tgross35 force-pushed the control-f16-f128-feature branch 4 times, most recently from aa0135b to 869c8ce Compare August 3, 2024 06:01
Intrinsics marked with `arm_aeabi_alias = ...` were having the rest of
their attributes eaten. Add them back.
By moving the logic for which platforms get symbols to
`compiler_builtins` rather than rust-lang/rust, we can control where
symbols get enabled without relying on Cargo features. Using Cargo
features turned out to be a problem in [1].

This will help resolve errors like [2].

[1]: rust-lang/rust#128358
[2]: rust-lang/rust#128401
Change from `not(feature = "no-f16-f128")` to `f16_enabled` or
`f128_disabled`, as applicable.
Previously we were building the C versions of these symbols. Since we
added the Rust version and updated compiler builtins, these are no
longer available by default. This is unintentional, but it gives a
better indicator of which symbol versions are not actually provided by
the system.

Use the list of build failures to correct the list of platforms that do
not have `f16` symbols.
The `sys_available` gate was incorrect. Update it.
The latest version has a convenient `.unwrap()`. Increase the version so
we can use this.
@tgross35 tgross35 force-pushed the control-f16-f128-feature branch 2 times, most recently from 91b8469 to 4872d5e Compare August 3, 2024 06:05
Since there are more platforms that do not have symbols present, we need
to use `rustc_apfloat` for more conversion tests. Make use of the
fallback like other tests, and refactor so each test gets its own
function.

Previously we were testing both apfloat and system conversion methods
when possible. This changes to only test one or the other, depending on
whether or not the system version is available. This seems reasonable
because it is consistent with all other tests, but we should consider
updating all tests to check both at some point.

This also includes an adjustment of PowerPC configuration to account for
the linking errors at [1].

[1]: rust-lang#655
@tgross35
Copy link
Contributor Author

tgross35 commented Aug 3, 2024

Alright, I think this should be good to go. There were a handful of small changes needed to get CI passing again after the builtins update in rustc, there are more details in the commit messages.

@tgross35 tgross35 marked this pull request as ready for review August 3, 2024 06:16
@Amanieu Amanieu merged commit 1211d02 into rust-lang:master Aug 4, 2024
24 checks passed
@tgross35
Copy link
Contributor Author

tgross35 commented Aug 5, 2024

Thanks. @Amanieu could you do a release? #657

@tgross35 tgross35 deleted the control-f16-f128-feature branch August 5, 2024 00:09
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 5, 2024
This includes [1] which means we can remove the (nonworking)
configuration of `no-f16-f128`.

Fixes rust-lang#128401.

[1]: rust-lang/compiler-builtins#652
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 5, 2024
This includes [1] which means we can remove the (nonworking)
configuration of `no-f16-f128`.

Fixes rust-lang#128401.

[1]: rust-lang/compiler-builtins#652
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 5, 2024
Update `compiler-builtins` to 0.1.115

This includes [1] which means we can remove the (nonworking) configuration of `no-f16-f128`.

Fixes rust-lang#128401.

[1]: rust-lang/compiler-builtins#652
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 5, 2024
Update `compiler-builtins` to 0.1.115

This includes [1] which means we can remove the (nonworking) configuration of `no-f16-f128`.

Fixes rust-lang#128401.

[1]: rust-lang/compiler-builtins#652

try-job: dist-various-1
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 6, 2024
This includes [1] which means we can remove the (nonworking)
configuration of `no-f16-f128`.

Fixes rust-lang#128401.

[1]: rust-lang/compiler-builtins#652
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 6, 2024
This includes [1] which means we can remove the (nonworking)
configuration of `no-f16-f128`.

Fixes rust-lang#128401.

[1]: rust-lang/compiler-builtins#652
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 7, 2024
This includes [1] which means we can remove the (nonworking)
configuration of `no-f16-f128`.

Fixes rust-lang#128401.

[1]: rust-lang/compiler-builtins#652
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 7, 2024
Update `compiler-builtins` to 0.1.117

This includes [1] which means we can remove the (nonworking) configuration of `no-f16-f128`.

Fixes rust-lang#128401.

[1]: rust-lang/compiler-builtins#652

try-job: dist-various-1
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 7, 2024
Update `compiler-builtins` to 0.1.117

This includes [1] which means we can remove the (nonworking) configuration of `no-f16-f128`.

Fixes rust-lang#128401.

[1]: rust-lang/compiler-builtins#652

try-job: dist-various-1
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 8, 2024
Update `compiler-builtins` to 0.1.117

This includes [1] which means we can remove the (nonworking) configuration of `no-f16-f128`.

Fixes rust-lang#128401.

[1]: rust-lang/compiler-builtins#652

try-job: dist-various-1
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
Rollup merge of rust-lang#128691 - tgross35:update-builtins, r=Amanieu

Update `compiler-builtins` to 0.1.117

This includes [1] which means we can remove the (nonworking) configuration of `no-f16-f128`.

Fixes rust-lang#128401.

[1]: rust-lang/compiler-builtins#652

try-job: dist-various-1
@kleisauke
Copy link
Contributor

I noticed that wasm32 is missing from this switch case:

compiler-builtins/build.rs

Lines 275 to 288 in 3ad4d9c

let (f16_ok, f128_ok) = match target.arch.as_str() {
// `f16` and `f128` both crash <https://github.com/llvm/llvm-project/issues/94434>
"arm64ec" => (false, false),
// `f16` crashes <https://github.com/llvm/llvm-project/issues/50374>
"s390x" => (false, true),
// `f128` crashes <https://github.com/llvm/llvm-project/issues/96432>
"mips64" | "mips64r6" => (true, false),
// `f128` crashes <https://github.com/llvm/llvm-project/issues/101545>
"powerpc64" if &target.os == "aix" => (true, false),
// `f128` crashes <https://github.com/llvm/llvm-project/issues/41838>
"sparc" | "sparcv9" => (true, false),
// Most everything else works as of LLVM 19
_ => (true, true),
};

Was this omission intentional? I'm encountering the same build failures described here:

// Linking says "error: function signature mismatch: __extendhfsf2" and seems to
// think the signature is either `(i32) -> f32` or `(f32) -> f32`
|| target.starts_with("wasm32-")

when building for wasm32-unknown-emscripten and using -Zbuild-std. However, I was able to work around the issue by also including -Zbuild-std-features=compiler-builtins-no-f16-f128.

@tgross35
Copy link
Contributor Author

We do seem to build successfully in rust-lang/rust for this target so I’m not sure what the difference is. But it makes sense to omit wasm32 for now if it is causing failures, mind putting up a PR?

@kleisauke
Copy link
Contributor

I just opened (draft) PR #665 for this.

Veykril pushed a commit to ferrocene/ferrocene that referenced this pull request Aug 12, 2024
This includes [1] which means we can remove the (nonworking)
configuration of `no-f16-f128`.

Fixes rust-lang/rust#128401.

[1]: rust-lang/compiler-builtins#652

Ferrocene-backport-of: #128691
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.

4 participants