Enable f16 tests on x86 and x86-64 by tgross35 · Pull Request #128349 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation68 Commits2 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Since the compiler_builtins update 1, ABI bugs on x86 should be resolved. Enable tests for f16 on these platforms now.
f16 math functions (reliable_f16_math) are still excluded because there is an LLVM crash for powi 2.
try-job: dist-i586-gnu-i586-i686-musl
try-job: x86_64-apple-1
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
bors added a commit to rust-lang-ci/rust that referenced this pull request
Enable f16 on x86 and x86-64
Since the compiler_builtins update 1, ABI bugs on x86 should be resolved. Enable tests for f16 on these platforms now.
try-job: i686-gnu try-job: dist-i586-gnu-i586-i686-musl
These changes are pretty straightforward, so may as well request a review now as long as the CI turns green.
r? libs
This comment was marked as resolved.
r? @Noratrieb
Seems like you missed triagebot.toml 😆
Seems like you missed triagebot.toml 😆
☀️ Try build successful - checks-actions
Build commit: 1d3189c (1d3189c078ab5012c5df110171d9d5246d2ca7f5)
| @@ -267,7 +267,7 @@ impl f16 { |
|---|
| /// |
| /// ``` |
| /// #![feature(f16)] |
| /// # #[cfg(target_arch = "aarch64")] { // FIXME(f16_F128): rust-lang/rust#123885 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to not enable it on AArch64 too still?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong reason, just a slightly more straightforward cfg (gets copied around a lot) and to be consistent with the rest of the file and f128. The core doctests are more or less just for smoke, since std has the better tests and all the platform config logic. (Unfortunately there doesn't seem to be an easy way to share this config with core).
BUT this made me double check the other gates in this file, and I realize I forgot to restrict to only linux. Updated that.
Rebased after #128388. This won't include anything new since symbols still aren't available.
Given that the doc tests are just smoke tests, this seems fine to me. Arguably the aarch64 cfg is simpler as it doesn't require Linux, but it doesn't really matter. Sorry for blocking the build script change on the doctests, that build script change is clearly important^^.
@bors r+
📌 Commit 0a5d5ff has been approved by Noratrieb
It is now in the queue for this repository.
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
That's a good point, these could almost not be linux-only except for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054. I think I just need to figure out a better way to reuse the working platforms logic into both core and std since it's useful elsewhere.
Anyway no worries, thanks for taking a look!
This was referenced
Aug 22, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request
Enable f16 on x86 and x86-64
Since the compiler_builtins update 1, ABI bugs on x86 should be resolved. Enable tests for f16 on these platforms now.
try-job: i686-gnu try-job: dist-i586-gnu-i586-i686-musl
tgross35 added a commit to tgross35/rust that referenced this pull request
Enable f16 on x86 and x86-64
Since the compiler_builtins update 1, ABI bugs on x86 should be resolved. Enable tests for f16 on these platforms now.
try-job: i686-gnu try-job: dist-i586-gnu-i586-i686-musl
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\release\cargo.exe 2x #127883 in a row.
@bors retry
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
bors added a commit to rust-lang-ci/rust that referenced this pull request
Enable f16 tests on x86 and x86-64
Since the compiler_builtins update 1, ABI bugs on x86 should be resolved. Enable tests for f16 on these platforms now.
f16 math functions (reliable_f16_math) are still excluded because there is an LLVM crash for powi 2.
try-job: dist-i586-gnu-i586-i686-musl try-job: x86_64-apple-1
The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] miri test:false 4.465
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`
Caused by:
Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
local time: Sat, Aug 24, 2024 10:02:08 AM
network time: Sat, 24 Aug 2024 10:02:09 GMT
##[error]Process completed with exit code 1.
Post job cleanup.
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
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
You're kidding. Anybody else a bit tired of that issue? 😆
Chris Denton is investigating it, see cargo zulip.
Finished benchmarking commit (f167efa): comparison URL.
Overall result: no relevant changes - no action needed
@rustbot label: -perf-regression
Instruction count
This benchmark run did not return any relevant results for this metric.
Max RSS (memory usage)
Results (primary 5.9%)
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) | 5.9% | [5.9%, 5.9%] | 1 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | 5.9% | [5.9%, 5.9%] | 1 |
Cycles
Results (secondary -3.1%)
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) | - | - | 0 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | -3.1% | [-3.6%, -2.3%] | 5 |
| All ❌✅ (primary) | - | - | 0 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 750.628s -> 750.904s (0.04%)
Artifact size: 338.92 MiB -> 338.92 MiB (-0.00%)
Labels
CI spurious failure: target env msvc
`#![feature(f16)]`, `#![feature(f128)]`
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the library team, which will review and decide on the PR/issue.