-
Notifications
You must be signed in to change notification settings - Fork 13.7k
fix(compiler/rustc_codegen_llvm): apply target-cpu
attribute
#145275
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
fix(compiler/rustc_codegen_llvm): apply target-cpu
attribute
#145275
Conversation
let mut attrs = SmallVec::<[_; 2]>::new(); | ||
|
||
let target_cpu = llvm_util::target_cpu(tcx.sess); | ||
let target_cpu_attr = llvm::CreateAttrStringValue(cx.llcx, "target-cpu", target_cpu); |
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.
https://doc.rust-lang.org/beta/nightly-rustc/rustc_codegen_llvm/context/struct.GenericCx.html#method.apply_target_cpu_attr
I would like to use this, but my cx
is SimpleCx
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.
You can move the method onto SimpleCx, it doesn't appear to use anything FullCx related
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.
I'm not very familiar with the code base. What is the recommended way to do this? There is MiscCodegenMethods
, but it probably has more methods.
This comment has been minimized.
This comment has been minimized.
Would it be possible to add a test for this? Either a codegen test or something in |
Some changes occurred in src/tools/compiletest cc @jieyouxu These commits modify the If this was unintentional then you should revert the changes before this PR is merged. The run-make-support library was changed cc @jieyouxu |
@alexcrichton done |
This comment has been minimized.
This comment has been minimized.
The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging. |
This comment has been minimized.
This comment has been minimized.
@@ -0,0 +1,25 @@ | |||
//@ only-wasm32v1-none |
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.
Oh I'm realizing now, does this actually run anywhere in CI? I don't believe tests are run for this target)
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.
Well, I ran the tests and looked at the output via wasm-dis | less
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.
Since wasm32v1-none
is a Tier2 target, it should probably be tested before publishing binaries: https://doc.rust-lang.org/nightly/rustc/platform-support.html#tier-2-without-host-tools.
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.
Would it be possible to rephrase this test in terms of minicore
in the test suite? I'm not sure if that's integrated into the run-make test suite though. In any case the main value from a test like this will be preventing regressions, but if it's not run on CI then it won't prevent regressions. If it's not run on CI I don't think it's worth adding a test.
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.
Hmm, what if we do the same test, but for wasm32-unknown-unknown
target? This test will use -Z build-std
and -C target-feature
, and it will also need the rust-src
component.
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.
@alexcrichton done
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.
Unfortunately AFAIK -Zbuild-std
is a known pain point in tests right now as it can take an excessively long amount of time to build and requires a lot of dependencies. I've seen comments in the past about how -Zbuild-std
should be avoided in tests.
I don't see existing integration with minicore
in run-make tests right now unfortunately. There is one test referencing it but the test's FIXME has not yet been updated.
Instead of using -Zbuild-std
could this build tests/auxiliary/minicore.rs
directly perhaps? I'll also cc @jieyouxu as you might know better how best to integrate minicore bits here than I
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.
Ok, I basically understand what you want. You want to use minicore
instead of compiling core
with the required features. But what should I do with alloc
?
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.
I will also need to somehow link any symbol marked as #[rustc_std_internal_symbol]
with #![no_core]
...
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 comment has been minimized.
This comment has been minimized.
24b0b81
to
f0c8714
Compare
f0c8714
to
f978932
Compare
The PR is mostly ready, but I would like someone to run |
@bors try jobs=test-various |
This comment has been minimized.
This comment has been minimized.
…try> fix(compiler/rustc_codegen_llvm): apply `target-cpu` attribute try-job: test-various
https://github.com/rust-lang/rust/actions/runs/16962093107/job/48077217291
|
@bors: r+ |
…1-none, r=alexcrichton fix(compiler/rustc_codegen_llvm): apply `target-cpu` attribute Resolves rust-lang#140174 r? `@alexcrichton` try-job: `test-various*`
Rollup of 22 pull requests Successful merges: - #118087 (Add Ref/RefMut try_map method) - #122661 (Change the desugaring of `assert!` for better error output) - #140740 (Add `-Zindirect-branch-cs-prefix`) - #142640 (Implement autodiff using intrinsics) - #143075 (compiler: Allow `extern "interrupt" fn() -> !`) - #144865 (Fix tail calls to `#[track_caller]` functions) - #144944 (E0793: Clarify that it applies to unions as well) - #144947 (Fix description of unsigned `checked_exact_div`) - #145004 (Couple of minor cleanups) - #145005 (strip prefix of temporary file names when it exceeds filesystem name length limit) - #145012 (Tail call diagnostics to include lifetime info) - #145065 (resolve: Introduce `RibKind::Block`) - #145120 (llvm: Accept new LLVM lifetime format) - #145189 (Weekly `cargo update`) - #145235 (Minor `[const]` tweaks) - #145275 (fix(compiler/rustc_codegen_llvm): apply `target-cpu` attribute) - #145322 (Resolve the prelude import in `build_reduced_graph`) - #145331 (Make std use the edition 2024 prelude) - #145369 (Do not ICE on private type in field of unresolved struct) - #145378 (Add `FnContext` in parser for diagnostic) - #145389 ([rustdoc] Revert "rustdoc search: prefer stable items in search results") - #145392 (coverage: Remove intermediate data structures from mapping creation) r? `@ghost` `@rustbot` modify labels: rollup
…1-none, r=alexcrichton fix(compiler/rustc_codegen_llvm): apply `target-cpu` attribute Resolves rust-lang#140174 r? ``@alexcrichton`` try-job: `test-various*`
Rollup of 21 pull requests Successful merges: - #118087 (Add Ref/RefMut try_map method) - #122661 (Change the desugaring of `assert!` for better error output) - #142640 (Implement autodiff using intrinsics) - #143075 (compiler: Allow `extern "interrupt" fn() -> !`) - #144865 (Fix tail calls to `#[track_caller]` functions) - #144944 (E0793: Clarify that it applies to unions as well) - #144947 (Fix description of unsigned `checked_exact_div`) - #145004 (Couple of minor cleanups) - #145005 (strip prefix of temporary file names when it exceeds filesystem name length limit) - #145012 (Tail call diagnostics to include lifetime info) - #145065 (resolve: Introduce `RibKind::Block`) - #145120 (llvm: Accept new LLVM lifetime format) - #145189 (Weekly `cargo update`) - #145235 (Minor `[const]` tweaks) - #145275 (fix(compiler/rustc_codegen_llvm): apply `target-cpu` attribute) - #145322 (Resolve the prelude import in `build_reduced_graph`) - #145331 (Make std use the edition 2024 prelude) - #145369 (Do not ICE on private type in field of unresolved struct) - #145378 (Add `FnContext` in parser for diagnostic) - #145389 ([rustdoc] Revert "rustdoc search: prefer stable items in search results") - #145392 (coverage: Remove intermediate data structures from mapping creation) r? `@ghost` `@rustbot` modify labels: rollup
…alexcrichton fix(tests/rmake/wasm-unexpected-features): change features from `WASM1` to `MVP` missed this in rust-lang#145275 since test calls `rustc` with `-C target-cpu mvp` try-job: `test-various`
Resolves #140174
r? @alexcrichton
try-job:
test-various*