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.

Sign up for GitHub

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

Jump to bottom

Enable f16 tests on x86 and x86-64 #128349

Merged
merged 2 commits into from
Aug 24, 2024
Merged

Enable f16 tests on x86 and x86-64 #128349

merged 2 commits into from
Aug 24, 2024

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jul 29, 2024 •
edited
Loading

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

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

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 29, 2024
@bors
Auto merge of rust-lang#128349 - tgross35:x86-f16, r=<try>
1d3189c 
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.

[1]: rust-lang#125016

try-job: i686-gnu
try-job: dist-i586-gnu-i586-i686-musl
@bors
Copy link
Contributor

bors commented Jul 29, 2024

⌛ Trying commit bd348e0 with merge 1d3189c...

@tgross35 tgross35 marked this pull request as ready for review July 29, 2024 16:06
@tgross35
Copy link
Contributor Author

These changes are pretty straightforward, so may as well request a review now as long as the CI turns green.

r? libs

@rustbot

This comment was marked as resolved.

Sign in to view
@tgross35
Copy link
Contributor Author

r? @Noratrieb

Seems like you missed triagebot.toml 😆

@jieyouxu
Copy link
Member

Seems like you missed triagebot.toml 😆

#128355

@bors
Copy link
Contributor

bors commented Jul 29, 2024

☀️ Try build successful - checks-actions
Build commit: 1d3189c (1d3189c078ab5012c5df110171d9d5246d2ca7f5)

Noratrieb
Noratrieb reviewed Jul 30, 2024
View reviewed changes
library/core/src/num/f16.rs
@@ -267,7 +267,7 @@ impl f16 {
///
/// ```
/// #![feature(f16)]
/// # #[cfg(target_arch = "aarch64")] { // FIXME(f16_F128): rust-lang/rust#123885
Copy link
Member

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?

Copy link
Contributor Author

@tgross35 tgross35 Jul 30, 2024 •
edited
Loading

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.

@bors
Copy link
Contributor

bors commented Jul 31, 2024

☔ The latest upstream changes (presumably #128435) made this pull request unmergeable. Please resolve the merge conflicts.

@tgross35
Copy link
Contributor Author

Rebased after #128388. This won't include anything new since symbols still aren't available.

@tgross35
Copy link
Contributor Author

Psst @Noratrieb can I get your okay here (I would like this for testing), or do you feel strongly about #128349 (comment)?

@tgross35 tgross35 added the F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` label Aug 12, 2024
@Noratrieb
Copy link
Member

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+

@bors
Copy link
Contributor

bors commented Aug 21, 2024

📌 Commit 0a5d5ff has been approved by Noratrieb

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 Aug 21, 2024
@tgross35
Copy link
Contributor Author

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
Closed
Open
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 22, 2024
@jieyouxu
Rollup merge of rust-lang#128349 - tgross35:x86-f16, r=Noratrieb
c7afff3 
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.

[1]: rust-lang#125016

try-job: i686-gnu
try-job: dist-i586-gnu-i586-i686-musl
@jieyouxu jieyouxu mentioned this pull request Aug 22, 2024
Closed
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 22, 2024
@tgross35
Rollup merge of rust-lang#128349 - tgross35:x86-f16, r=Noratrieb
b986d9b 
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.

[1]: rust-lang#125016

try-job: i686-gnu
try-job: dist-i586-gnu-i586-i686-musl
@tgross35 tgross35 mentioned this pull request Aug 22, 2024
Closed
@rust-log-analyzer
Copy link
Collaborator

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)

@bors
Copy link
Contributor

bors commented Aug 24, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review 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 Aug 24, 2024
@tgross35
Copy link
Contributor Author

tgross35 commented Aug 24, 2024 •
edited
Loading

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

@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 Aug 24, 2024
@bors
Copy link
Contributor

bors commented Aug 24, 2024

⌛ Testing commit e76b7d0 with merge 1a5afdd...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 24, 2024
@bors
Auto merge of rust-lang#128349 - tgross35:x86-f16, r=Noratrieb
1a5afdd 
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].

[1]: rust-lang#125016
[2]: llvm/llvm-project#105747

try-job: dist-i586-gnu-i586-i686-musl
try-job: x86_64-apple-1
@rust-log-analyzer
Copy link
Collaborator

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.

@bors
Copy link
Contributor

bors commented Aug 24, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review 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 Aug 24, 2024
@jieyouxu
Copy link
Member

Congrats! 3x #127883 in a row
@bors retry

@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 Aug 24, 2024
@Noratrieb
Copy link
Member

🪙 <- your prize

@tgross35
Copy link
Contributor Author

You're kidding. Anybody else a bit tired of that issue? 😆

@Noratrieb
Copy link
Member

Chris Denton is investigating it, see cargo zulip.

@tgross35
Copy link
Contributor Author

Chris Denton is investigating it, see cargo zulip.

Seen :) #127883 (comment)

@bors
Copy link
Contributor

bors commented Aug 24, 2024

⌛ Testing commit e76b7d0 with merge f167efa...

@bors
Copy link
Contributor

bors commented Aug 24, 2024

☀️ Test successful - checks-actions
Approved by: Noratrieb
Pushing f167efa to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 24, 2024
@bors bors merged commit f167efa into rust-lang:master Aug 24, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 24, 2024
@Noratrieb
Copy link
Member

congrats!!!!

@jieyouxu
Copy link
Member

confetti sounds

@rust-timer
Copy link
Collaborator

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%)

@tgross35
Copy link
Contributor Author

Sixth time’s the charm!

@tgross35 tgross35 deleted the x86-f16 branch August 24, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Noratrieb Noratrieb Noratrieb left review comments

@beetrees beetrees beetrees left review comments

Assignees

@Noratrieb Noratrieb

Labels
F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` 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-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.82.0
Development

Successfully merging this pull request may close these issues.
8 participants
@tgross35 @bors @rustbot @jieyouxu @Noratrieb @rust-log-analyzer @beetrees @rust-timer