Skip to content

Only run asm tests if they match the current codegen backend #144249

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jul 21, 2025

Follow-up of #144125.

This PR changes compiletest so that asm tests are only run if they match the current codegen backend. To better reflect it, I renamed the tests/ui/asm folder into tests/ui/asm-llvm. Like that, we can add new asm tests for other backends if we want without needing to add extra code to compiletest.

Next step will be to use the new code annotations added in #144125 to ignore ui tests failing in cg_gcc until it's fixed on our side.

cc @antoyo @oli-obk
r? @Kobzol

@rustbot rustbot added A-compiletest Area: The compiletest test runner 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 21, 2025

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the A-tidy Area: The tidy tool label Jul 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 21, 2025

There are changes to the tidy tool.

cc @jieyouxu

@rustbot
Copy link
Collaborator

rustbot commented Jul 21, 2025

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol, @tshepang

@rustbot rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Jul 21, 2025
@Kobzol
Copy link
Member

Kobzol commented Jul 21, 2025

Since this is mostly a compiletest thing, I'll delegate to jieyouxu: Maybe there's a better way to detect the asm directory in the UI test suite? Btw we should do something similar for tests/assembly and tests/codegen, right?

r? @jieyouxu

@rustbot rustbot assigned jieyouxu and unassigned Kobzol Jul 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 21, 2025

jieyouxu is not on the review rotation at the moment.
They may take a while to respond.

Copy link
Member

@bjorn3 bjorn3 Jul 21, 2025

Choose a reason for hiding this comment

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

These tests all are not LLVM specific. They test asm!() (which is backend agnostic, just target dependent), not --emit asm (which will produce different output depending on the backend).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah interesting! So I should move all asm!() tests into a tests/ui/asm folder which would be backend agnostic.

One question: should I move all tests containing asm! calls or is there another heuristic?

Copy link
Member

Choose a reason for hiding this comment

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

tests/ui/asm shouldn't contain any backend specific tests. Those are in tests/assembly and tests/codegen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see, sorry I misunderstood. Then updating the other testsuites.

Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

^

@rustbot rustbot added A-CI Area: Our Github Actions CI A-meta Area: Issues & PRs about the rust-lang/rust repository itself T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jul 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 21, 2025

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in src/tools/opt-dist

cc @Kobzol

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@GuillaumeGomez
Copy link
Member Author

Applied suggestions.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Added the missing bits so I think I didn't forget anything this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI Area: Our Github Actions CI A-compiletest Area: The compiletest test runner A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-rustc-dev-guide Area: rustc-dev-guide A-testsuite Area: The testsuite used to check the correctness of rustc A-tidy Area: The tidy tool 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants