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

Test that target feature mix up with homogeneous floats is sound #101381

Merged
merged 1 commit into from
Nov 9, 2022
Merged

Test that target feature mix up with homogeneous floats is sound #101381

merged 1 commit into from
Nov 9, 2022

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Sep 3, 2022

This pull-request adds a test in src/test/abi/ that test that target feature mix up with homogeneous floats is sound.

This is basically is ripoff of src/test/ui/simd/target-feature-mixup.rs but for floats and without #[repr(simd)].

Extracted from #97559 since I don't yet know what to do with that PR.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 3, 2022
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 3, 2022
@Mark-Simulacrum
Copy link
Member

r? @Amanieu perhaps?

I'm not a huge fan of just running binaries without checking CPUID ahead of time for the relevant target features. I think our CI runs on fairly old CPUs (Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz, in one recent job), which don't have any AVX-512 features, so we'd not be testing this regularly either.

@Amanieu
Copy link
Member

Amanieu commented Sep 5, 2022

Fundamentally this LGTM. If we absolutely want to make sure that the AVX512 path is tested then we may want to run the test under qemu-x86_64.

@Urgau
Copy link
Member Author

Urgau commented Sep 18, 2022

Fundamentally this LGTM. If we absolutely want to make sure that the AVX512 path is tested then we may want to run the test under qemu-x86_64.

Is is required ? Personally I don't it should be, qemu is not perfect and most contributors will run those tests on their machines, so it will get tested as much but still enough to get me to want to integrate it. Having the test is better than not having it at all.

@Amanieu
Copy link
Member

Amanieu commented Sep 18, 2022

No, I don't think this is strictly required. This test is fine as it is.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 18, 2022

📌 Commit 5c6caf3 has been approved by Amanieu

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 Sep 18, 2022
@bors
Copy link
Contributor

bors commented Sep 19, 2022

⌛ Testing commit 5c6caf3 with merge 97c338f2fa6662ed23b3cd8d295a529a8a14487f...

@bors
Copy link
Contributor

bors commented Sep 19, 2022

💔 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 Sep 19, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
notriddle added a commit to notriddle/rust that referenced this pull request Sep 20, 2022
@notriddle
Rollup merge of rust-lang#101381 - Urgau:target-mixup-homogenous-floa…
2beae13 
…ts, r=Amanieu

Test that target feature mix up with homogeneous floats is sound

This pull-request adds a test in `src/test/abi/` that test that target feature mix up with homogeneous floats is sound.

This is basically is ripoff of [src/test/ui/simd/target-feature-mixup.rs](https://github.com/rust-lang/rust/blob/47d1cdb0bcac8e417071ce1929d261efe2399ae2/src/test/ui/simd/target-feature-mixup.rs) but for floats and without `#[repr(simd)]`.

*Extracted from rust-lang#97559 since I don't yet know what to do with that PR.*
@notriddle notriddle mentioned this pull request Sep 20, 2022
Closed
@notriddle
Copy link
Contributor

Wait, never mind. This one failed in CI and wasn't fixed.

I wonder how it got back into the bors queue?

@notriddle
Copy link
Contributor

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 20, 2022
@Urgau
Copy link
Member Author

Urgau commented Sep 20, 2022

Wait, never mind. This one failed in CI and wasn't fixed.

I wonder how it got back into the bors queue?

Not the first time. It sometimes happens that the queue gets de-synchronized.

Anyway, I looked a bit at the failure and it happen when testing the first sse2 assertion on i586-unknown-linux-gnu. I tried to replicate the failure but didn't yet succeed.

It's maybe related to sse not being available on CI but it would be weird to not get a SIGGILL in that case. Anyone know if the builder dist-i586-gnu-i586-i686-musl uses qemu or something else ?

@Amanieu
Copy link
Member

Amanieu commented Sep 20, 2022

The i586-unknown-linux-gnu target specifically doesn't enable the sse target feature by default. This means that it passes floating-point arguments via the x87 registers instead of using SSE registers. I expect this might be what is causing the issue here.

@Urgau
Copy link
Member Author

Urgau commented Sep 28, 2022

I tried pretty hard to reproduce the crash (with/without qemu, with/without mold, with/without native libs, ...) but I'm still unable to reproduce it. It always successfully pass.

I'm not sure what to do or try anymore. Does anyone have an idea?

@Amanieu
Copy link
Member

Amanieu commented Sep 29, 2022

Can you reproduce the issue with ./x.py test src/test/ui --target=i586-unknown-linux-gnu?

@Urgau
Copy link
Member Author

Urgau commented Sep 29, 2022

Can you reproduce the issue with ./x.py test src/test/ui --target=i586-unknown-linux-gnu?

Seems like yes, which does not really make sense to me. Anyway this is progress. I will try to investigate this week-end otherwise next week.

@Urgau
Test that target feature mix up with homogeneous floats is sound
66847ff 
This is basically is ripoff of src/test/ui/simd/target-feature-mixup.rs
but for floats and without #[repr(simd)]
@Urgau Urgau force-pushed the target-mixup-homogenous-floats branch from 5c6caf3 to 66847ff Compare November 1, 2022 13:22
@Urgau
Copy link
Member Author

Urgau commented Nov 1, 2022

I've been unable to find the exact cause of the problem but since src/test/ui/see2.rs manually disable any sse2 test on i586-unknown-linux-gnu, I've done the same and added the same check.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 1, 2022
@Urgau Urgau closed this Nov 1, 2022
@Urgau Urgau reopened this Nov 1, 2022
@Amanieu
Copy link
Member

Amanieu commented Nov 9, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Nov 9, 2022

📌 Commit 66847ff has been approved by Amanieu

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 9, 2022
@Manishearth Manishearth mentioned this pull request Nov 9, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2022
@bors
Auto merge of rust-lang#104179 - Manishearth:rollup-yvsx5hh, r=Manish…
91385d5 
…earth

Rollup of 7 pull requests

Successful merges:

 - rust-lang#100508 (avoid making substs of type aliases late bound when used as fn args)
 - rust-lang#101381 (Test that target feature mix up with homogeneous floats is sound)
 - rust-lang#103353 (Fix Access Violation when using lld & ThinLTO on windows-msvc)
 - rust-lang#103521 (Avoid possible infinite  loop when next_point reaching the end of file)
 - rust-lang#103559 (first move on a nested span_label)
 - rust-lang#103778 (Update several crates for improved support of the new targets)
 - rust-lang#103827 (Properly remap and check for substs compatibility in `confirm_impl_trait_in_trait_candidate`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bd4e608 into rust-lang:master Nov 9, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Amanieu Amanieu

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Urgau @rust-highfive @Mark-Simulacrum @Amanieu @bors @rust-log-analyzer @notriddle @rustbot