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

Use an explicit x86-64 cpu in tests that are sensitive to it #124597

Merged
merged 3 commits into from
May 2, 2024

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented May 1, 2024

There are a few tests that depend on some target features not being
enabled by default, and usually they are correct with the default x86-64
target CPU. However, in downstream builds we have modified the default
to fit our distros -- x86-64-v2 in RHEL 9 and x86-64-v3 in RHEL 10
-- and the latter especially trips tests that expect not to have AVX.

These cases are few enough that we can just set them back explicitly.

There are a few tests that depend on some target features **not** being
enabled by default, and usually they are correct with the default x86-64
target CPU. However, in downstream builds we have modified the default
to fit our distros -- `x86-64-v2` in RHEL 9 and `x86-64-v3` in RHEL 10
-- and the latter especially trips tests that expect not to have AVX.

These cases are few enough that we can just set them back explicitly.
@rustbot
Copy link
Collaborator

rustbot commented May 1, 2024

r? @davidtwco

rustbot has assigned @davidtwco.
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 S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 1, 2024
tests/assembly/simd-intrinsic-mask-reduce.rs Show resolved Hide resolved
tests/assembly/x86_64-floating-point-clamp.rs Show resolved Hide resolved
tests/ui/sse2.rs Outdated
Comment on lines 23 to 24
assert!(cfg!(not(target_feature = "avx512f")),
"AVX512 shouldn't be detected as available by default on any platform");
Copy link
Member

Choose a reason for hiding this comment

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

hmm, if we add a v4 platform this is not the case... why not name a really outlandish target feature, like "avx512vp2intersect"? it is now fully deprecated in Intel CPUs, i.e. none of their newer SKUs support them, no matter what segment, so we can expect it to never make it into a platform default. ever.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, although I wonder, does this need to be a real feature at all? This test runs on non-x86 targets too, and it seems not to mind if I put something like "ferris_wheel" in there either.

Copy link
Member Author

Choose a reason for hiding this comment

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

I put in my silly one, but I can switch to your deprecated feature if preferred for some reason.

Copy link
Member

Choose a reason for hiding this comment

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

No that's great. :3

@workingjubilee
Copy link
Member

Oh, I don't think RHEL shouldn't upstream it, I just felt the patch may not survive someone "tidying up" and a reviewer not looking too closely at the blame, unless it explained itself~

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 2, 2024

📌 Commit 393d933 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 May 2, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 2, 2024
…llaumeGomez

Rollup of 3 pull requests

Successful merges:

 - rust-lang#124568 (Adjust `#[macro_export]`/doctest help suggestion for non_local_defs lint)
 - rust-lang#124582 (always print nice 'std not found' error when std is not found)
 - rust-lang#124597 (Use an explicit x86-64 cpu in tests that are sensitive to it)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1b0972c into rust-lang:master May 2, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 2, 2024
Rollup merge of rust-lang#124597 - cuviper:x86-64-v3, r=workingjubilee

Use an explicit x86-64 cpu in tests that are sensitive to it

There are a few tests that depend on some target features **not** being
enabled by default, and usually they are correct with the default x86-64
target CPU. However, in downstream builds we have modified the default
to fit our distros -- `x86-64-v2` in RHEL 9 and `x86-64-v3` in RHEL 10
-- and the latter especially trips tests that expect not to have AVX.

These cases are few enough that we can just set them back explicitly.
@rustbot rustbot added this to the 1.80.0 milestone May 2, 2024
@workingjubilee workingjubilee added the O-x86_64 Target: x86-64 processors (like x86_64-*) label Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-x86_64 Target: x86-64 processors (like x86_64-*) 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
Development

Successfully merging this pull request may close these issues.

5 participants