-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Do not run ui test if options specific to LLVM are used when another codegen backend is used #146618
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
Do not run ui test if options specific to LLVM are used when another codegen backend is used #146618
Conversation
Some changes occurred in src/tools/compiletest cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
f2b78ae
to
2c13435
Compare
And fixed fmt. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left two nits.
name: "needs-enzyme", | ||
condition: config.has_enzyme, | ||
condition: config.has_enzyme && config.default_codegen_backend.is_llvm(), | ||
ignore_reason: "ignored when LLVM Enzyme is disabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore_reason: "ignored when LLVM Enzyme is disabled", | |
ignore_reason: "ignored when LLVM Enzyme is disabled or LLVM is not the default codegen backend", |
name: "needs-llvm-zstd", | ||
condition: cache.llvm_zstd, | ||
condition: cache.llvm_zstd && config.default_codegen_backend.is_llvm(), | ||
ignore_reason: "ignored if LLVM wasn't build with zstd for ELF section compression", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore_reason: "ignored if LLVM wasn't build with zstd for ELF section compression", | |
ignore_reason: "ignored if LLVM wasn't build with zstd for ELF section compression or LLVM is not the default codegen backend", |
…codegen backend is used
2c13435
to
a535042
Compare
Done! @bors r=kobzol rollup |
Rollup of 5 pull requests Successful merges: - #146442 (Display ?Sized, const, and lifetime parameters in trait item suggestions across a crate boundary) - #146474 (Improve `core::ascii` coverage) - #146605 (Bump rustfix 0.8.1 -> 0.8.7) - #146611 (bootstrap: emit hint if a config key is used in the wrong section) - #146618 (Do not run ui test if options specific to LLVM are used when another codegen backend is used) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #146618 - GuillaumeGomez:backend-run-llvm-options, r=kobzol Do not run ui test if options specific to LLVM are used when another codegen backend is used Based on errors in #146414, some tests with LLVM-specific options are run when another codegen is actually the one used. This PR ignores these tests in such cases now to prevent this situation. r? `@kobzol`
if config.default_codegen_backend.is_llvm() { | ||
return IgnoreDecision::Continue; | ||
} | ||
return IgnoreDecision::Ignore { reason: "LLVM specific test".into() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct. needs-llvm-components
is only informational so that the test is elided if the target cannot be built for using the available LLVM components. Many of these are x86 targets that we should be able to test using cg_gcc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In particular, needs-llvm-components is commonly used for gnarly target-specific tests that we set up with no_core so they can run on all targets; we then need to tell compiletest about that so that it knows whether building for this target is possible at all. None of these are backend-specific.
Those are never "run" tests since obviously we can't run random binaries for a different target, so most of codegen is not relevant. However, tests like tests/ui/target-feature/abi-incompatible-target-feature-flag-enable.rs
and its brethren are still crucial to run for all codegen backends since a bunch of the target feature logic lives inside the backends.
Based on errors in #146414, some tests with LLVM-specific options are run when another codegen is actually the one used.
This PR ignores these tests in such cases now to prevent this situation.
r? @Kobzol