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

Update compiler-builtins and enable f128 tests on all non-buggy platforms #132434

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Nov 1, 2024

Update compiler_builtins to 0.1.138 and pin it. This updates to a new version of builtins that includes 1, which was
the last blocker to us enabling f128 tests on all platforms.

With that, we now provide symbols necessary to work with f128 everywhere. This means that we are no longer restricted to systems that provide f128 symbols themselves, and can enable tests by default.

There are still a handful of platforms that need to remain disabled because of bugs and some that had to get updated.

Math support is still off by default since those symbols are not yet available.

try-job: test-various
try-job: i686-gnu-nopt

@rustbot
Copy link
Collaborator

rustbot commented Nov 1, 2024

r? @workingjubilee

rustbot has assigned @workingjubilee.
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 A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 1, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 1, 2024

These commits modify the library/Cargo.lock file. Unintentional changes to library/Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

@tgross35
Copy link
Contributor Author

tgross35 commented Nov 1, 2024

This includes #132433 which includes #132206, so the chain is a bit deep (I'm keeping updates separate from rust-lang/rust changes so we aren't blocked from updating further if something goes wrong here). But that doesn't mean we can't start some tests.

@bors try

@tgross35
Copy link
Contributor Author

tgross35 commented Nov 1, 2024

Those are quite a few try jobs but it still doesn't cover everything that will be newly enabled here.

@bors rollup=never

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 1, 2024
Enable f128 tests on all non-buggy platforms 🎉

With the `compiler-builtins` update to 0.1.137 [1], we now provide symbols necessary to work with `f128` everywhere. This means that we are no longer restricted to 64-bit linux, and can enable tests by default.

There are still a handful of platforms that need to remain disabled because of bugs.

Math support is still off by default since those symbols are not yet available.

[1]: rust-lang#132433

try-job: arm-android
try-job: armhf-gnu
try-job: i686-gnu
try-job: x86_64-apple-1
try-job: i686-mingw
try-job: x86_64-msvc-ext
@bors
Copy link
Contributor

bors commented Nov 1, 2024

⌛ Trying commit 15075bf with merge ec0220a...

@tgross35
Copy link
Contributor Author

tgross35 commented Nov 1, 2024

Also cc @beetrees

@bors
Copy link
Contributor

bors commented Nov 1, 2024

☀️ Try build successful - checks-actions
Build commit: ec0220a (ec0220aadb3398fa8aa480a8d4a0f8e3e6d48e20)

@workingjubilee
Copy link
Member

This has a merge conflict, it seems.

@workingjubilee
Copy link
Member

I have rebased it.

@beetrees
Copy link
Contributor

beetrees commented Nov 3, 2024

I think the f128 tests need to be disabled on 32-bit x86 due to llvm/llvm-project#77401 (although tests might pass anyway if the symbols from the LLVM-compiled compiler-builtins get picked by the linker instead of those from a GCC-compiled system library).

Also mips64/mips64r6 currently doesn't have f128 builtins built for it due to llvm/llvm-project#96432.

@workingjubilee
Copy link
Member

workingjubilee commented Nov 3, 2024

I do not believe we run the relevant tests on mips64?

@workingjubilee
Copy link
Member

I decided to change up the suite of tests slightly and rerun this. In particular, I noticed we didn't run the infamous test-various.

@bors try

@bors
Copy link
Contributor

bors commented Nov 3, 2024

⌛ Trying commit 95998ee with merge ad17ccb...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 3, 2024
Enable f128 tests on all non-buggy platforms 🎉

With the `compiler-builtins` update to 0.1.137 [1], we now provide symbols necessary to work with `f128` everywhere. This means that we are no longer restricted to 64-bit linux, and can enable tests by default.

There are still a handful of platforms that need to remain disabled because of bugs.

Math support is still off by default since those symbols are not yet available.

[1]: rust-lang#132433

try-job: armhf-gnu
try-job: i686-gnu
try-job: i686-gnu-nopt
try-job: test-various
try-job: dist-mips-linux
try-job: dist-mips64el-linux
@rust-log-analyzer

This comment has been minimized.

@beetrees
Copy link
Contributor

beetrees commented Nov 3, 2024

I do not believe we run the relevant tests on mips64?

All MIPS targets are tier 3, so no builds or tests of any kind are run by rust-lang/rust. The benefit of adding it to the match statement is just for those running tests locally/Linux distros with a MIPS64 port etc. 32-bit sparc is also tier 3 only and is already on the list.

@beetrees
Copy link
Contributor

beetrees commented Nov 3, 2024

Mention of #125102 can be removed from the PowerPC arm of the match statement as that issue is now fixed. The other PowerPC-related bugs currently unfixed that aren't currently listed in the match arm are llvm/llvm-project#98126, llvm/llvm-project#92866 (powerpc64 little-endian only), and llvm/llvm-project#101545 (powerpc64-ibm-aix only); since there are four open issues in total linking to the f16/f128 tracking issue (#116909) instead of listing them all in the source code individually also seems reasonable.

This updates to a new version of builtins that includes [1], which was
the last blocker to us enabling `f128` tests on all platforms 🎉.

With this update, also change to pinning the version with `=` rather
than using the default carat versioning. This is meant to ensure that
`compiler-builtins` does not get updated as part of the weekly
`Cargo.lock` update, since updates to this crate need to be intentional:
changes to rust-lang/rust and rust-lang/compiler-builtins sometimes need
to be kept in lockstep, unlike most dependencies, and sometimes these
updates can be problematic.

[1]: rust-lang/compiler-builtins#624
@tgross35
Copy link
Contributor Author

tgross35 commented Nov 4, 2024

I updated this to skip x86 and mips as mentioned. Indeed we don't test this (and try job only allow the jobs listed here, though I wish it was possible to do try jobs for tier 3 targets) but I know there are people running the test suite locally, e.g. #127235.

Also adjusted some comments.

Updated try jobs to those that have been mentioned but not yet tested.

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2024
Enable f128 tests on all non-buggy platforms 🎉

With the `compiler-builtins` update to 0.1.137 [1], we now provide symbols necessary to work with `f128` everywhere. This means that we are no longer restricted to 64-bit linux, and can enable tests by default.

There are still a handful of platforms that need to remain disabled because of bugs.

Math support is still off by default since those symbols are not yet available.

[1]: rust-lang#132433

try-job: test-various
try-job: i686-gnu-nopt
@bors
Copy link
Contributor

bors commented Nov 4, 2024

⌛ Trying commit 898ecf0 with merge 2ec481c395b83f7d7eb0b3521f6ab25f23a6360d...

library/std/build.rs Outdated Show resolved Hide resolved
library/std/build.rs Outdated Show resolved Hide resolved
With the `compiler-builtins` update to 0.1.137 [1], we now provide
symbols necessary to work with `f128` everywhere. This means that we are
no longer restricted to 64-bit linux, and can enable tests by default.

There are still a handful of platforms that need to remain disabled
because of bugs. This patch additionally disables the following:

1. MIPS [2]
2. 32-bit x86 [3]

Math support is still off by default since those symbols are not yet
available.

[1]: rust-lang#132433
[2]: llvm/llvm-project#96432
[3]: llvm/llvm-project#77401
Copy link
Contributor

@beetrees beetrees left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@tgross35 tgross35 changed the title Enable f128 tests on all non-buggy platforms 🎉 Update compiler-builtins and enable f128 tests on all non-buggy platforms Nov 4, 2024
@workingjubilee
Copy link
Member

All MIPS targets are tier 3, so no builds or tests of any kind are run by rust-lang/rust. The benefit of adding it to the match statement is just for those running tests locally/Linux distros with a MIPS64 port etc. 32-bit sparc is also tier 3 only and is already on the list.

mmh. my usual inclination is that we should leave known-failing-on-tier-3 tests on, so that people who are doing just that are aware that the platform's support is incomplete... and if they run the tests and then are bothered by it, we're usually amenable to exclusions.

as this is a nightly feature though I'm not strongly motivated either way.

@workingjubilee
Copy link
Member

let's get this show on the road:
@bors r+

@bors
Copy link
Contributor

bors commented Nov 4, 2024

📌 Commit c0fc25c has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2024
@bors
Copy link
Contributor

bors commented Nov 4, 2024

⌛ Testing commit c0fc25c with merge 706eec8...

@bors
Copy link
Contributor

bors commented Nov 4, 2024

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 706eec8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 4, 2024
@bors bors merged commit 706eec8 into rust-lang:master Nov 4, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 4, 2024
@tgross35 tgross35 deleted the f128-tests branch November 4, 2024 06:47
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (706eec8): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.5% [1.5%, 1.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.4% [-1.4%, -1.4%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -1.2%, secondary 1.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.8% [1.2%, 4.8%] 3
Improvements ✅
(primary)
-1.2% [-1.2%, -1.2%] 1
Improvements ✅
(secondary)
-3.0% [-3.0%, -3.0%] 1
All ❌✅ (primary) -1.2% [-1.2%, -1.2%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 780.005s -> 778.282s (-0.22%)
Artifact size: 335.27 MiB -> 335.31 MiB (0.01%)

@jethrogb
Copy link
Contributor

jethrogb commented Nov 6, 2024

Is it correct that compiler-builtins/libm doesn't define fmodl (for f128)?

@jethrogb
Copy link
Contributor

jethrogb commented Nov 6, 2024

I see in your first message you mention:

Math support is still off by default since those symbols are not yet available.

However, the compiler does appear to be generating references to fmodl with this PR merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants