attach global target features to module-level assembly - #160594

folkertdev · 2026-08-05T22:22:02Z

I did not use an LLM to create a change in this PR.
I used an LLM to create a change in this PR, and I have explained below how it was used.

At long last, we can forward global target features to LLVM and it will preserve the target features a block of module-level assembly was defined with through LTO.

cc @nikic (who made this happen)
cc @RalfJung any nasty side-effects we might be overlooking here?

rustbot · 2026-08-05T22:22:07Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
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

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler
compiler expanded to 75 candidates
Random selection from 17 candidates

folkertdev · 2026-08-05T22:24:20Z

+  Props.TargetFeatures = StringRef(TargetFeatures, TargetFeaturesLen);
+  Props.TargetCPU = StringRef(TargetCPU, TargetCPULen);
+  unwrap(M)->appendModuleInlineAsm(
+      Module::GlobalAsmFragment(std::string(Asm, AsmLen), std::move(Props)));


the std::move is required here. It isn't there in some of the LLVM source, I'm not sure why, but llvm complains loudly without it.

I believe this needs a std::string instead of a StringRef because GlobalAsmFragment needs something owned. Hopefully this is right in terms of ownership and cleanup?

View changes since the review

the std::move is required here. It isn't there in some of the LLVM source, I'm not sure why, but llvm complains loudly without it.

Shouldn't really be required here, but the std::move is fine...

I believe this needs a std::string instead of a StringRef because GlobalAsmFragment needs something owned. Hopefully this is right in terms of ownership and cleanup?

Either would work, but it will become an owned std::string in the end, so construct it as such makes sense.

folkertdev · 2026-08-05T22:24:35Z

   |
 LL | .intel_syntax noprefix
   | ^
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`


no idea where this comes from

View changes since the review

Me neither...

-Z deduplicate-diagnostics=no is set by compiletest AFAIK.

The question is more why the diagnostic appears twice after this change :)

folkertdev · 2026-08-05T22:25:31Z

+        llvm::append_module_inline_asm(llmod, &asm, "", "");
        let asm = create_section_with_flags_asm(".llvmcmd", section_flags, &[]);
-        llvm::append_module_inline_asm(llmod, &asm);
+        llvm::append_module_inline_asm(llmod, &asm, "", "");


I believe we just embed bytes here, so the target features and cpu don't seem relevant.

View changes since the review

RalfJung · 2026-08-05T22:32:21Z

any nasty side-effects we might be overlooking here?

I don't have the slightest clue what it means to forward target features to assembly.^^

folkertdev · 2026-08-05T22:37:00Z

Fair. So what this does is attach the target features and cpu information to piece of assembly. Previously the target information would sometimes get lost, especially when doing LTO and potentially merging modules with different target features. LLVM would error when assembly used instructions that are gated on a particular target feature, even though the feature is in fact globally enabled.

My practical question for you is: am I forwarding the right target features here? and might a particular target cpu that we forward imply target features that we didn't intend to enable?

RalfJung · 2026-08-06T06:09:42Z

I have no idea, sorry. I don't even know which potential problems one has to be aware of here.

petrochenkov · 2026-08-06T10:30:17Z

r? @nikic

folkertdev · 2026-08-25T19:43:44Z

I've prepared a followup that also forwards target features on naked functions:

support #[target_feature(enable = ...)] on #[naked] functions #137720

nikic

Looks reasonable.

View changes since this review

nikic · 2026-08-26T07:53:01Z

+#if LLVM_VERSION_GE(23, 0)
+  Module::GlobalAsmProperties Props;
+  Props.TargetFeatures = StringRef(TargetFeatures, TargetFeaturesLen);
+  Props.TargetCPU = StringRef(TargetCPU, TargetCPULen);


Both of these are std::strings. It's probably more obvious to directly construct it instead of creating it from StringRef.

nikic · 2026-08-26T07:54:36Z

+  Props.TargetFeatures = StringRef(TargetFeatures, TargetFeaturesLen);
+  Props.TargetCPU = StringRef(TargetCPU, TargetCPULen);
+  unwrap(M)->appendModuleInlineAsm(
+      Module::GlobalAsmFragment(std::string(Asm, AsmLen), std::move(Props)));


the std::move is required here. It isn't there in some of the LLVM source, I'm not sure why, but llvm complains loudly without it.

Shouldn't really be required here, but the std::move is fine...

I believe this needs a std::string instead of a StringRef because GlobalAsmFragment needs something owned. Hopefully this is right in terms of ownership and cleanup?

Either would work, but it will become an owned std::string in the end, so construct it as such makes sense.

nikic · 2026-08-26T07:56:52Z

   |
 LL | .intel_syntax noprefix
   | ^
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`


Me neither...

rustbot · 2026-08-26T08:58:43Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

rustbot · 2026-08-26T11:11:52Z

rustc_codegen_cranelift is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_cranelift instead.

cc @bjorn3

rustc_codegen_gcc is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_gcc instead.

cc @antoyo, @GuillaumeGomez

…uwer Rollup of 16 pull requests Successful merges: - #161945 (std: optimise IO error formatting) - #160594 (attach global target features to module-level assembly) - #161577 (implement [u8]::split_ascii_whitespace) - #161858 (fix ICE in generic_const_parameter_types with inherents) - #161377 ([bootstrap] Don't reverse the order of dylib search path entries) - #161702 (Use `drop_guard` in some places in {core,alloc,std}) - #161804 (Document PartialOrd behavior for Option<T> where T: PartialOrd) - #161865 (loongarch: support passing `u128`/`i128` to inline assembly) - #161877 (Do not load macro metadata for local definitions in rustdoc) - #161880 (fix rustc_lint_defs doctest issues) - #161883 (better deal with internal features being injected into doctests) - #161887 (std: uefi: fix File::seek returning the EOF sentinel) - #161897 (Reject contract attributes without arguments) - #161909 (Report the configured Polonius default in -Z help) - #161910 (Add rustdoc-html regression test for generated macro) - #161914 (Retroactively add relnotes for `bool::{ok_or,ok_or_else}` (1.98.0))

@nikic

…ures, r=nikic attach global target features to module-level assembly fixes rust-lang#80608 fixes rust-lang#127269 At long last, we can forward global target features to LLVM and it will preserve the target features a block of module-level assembly was defined with through LTO. cc @nikic (who made this happen) cc @RalfJung any nasty side-effects we might be overlooking here?

Rollup of 17 pull requests Successful merges: - #161945 (std: optimise IO error formatting) - #160594 (attach global target features to module-level assembly) - #161577 (implement [u8]::split_ascii_whitespace) - #161858 (fix ICE in generic_const_parameter_types with inherents) - #161377 ([bootstrap] Don't reverse the order of dylib search path entries) - #161804 (Document PartialOrd behavior for Option<T> where T: PartialOrd) - #161865 (loongarch: support passing `u128`/`i128` to inline assembly) - #161877 (Do not load macro metadata for local definitions in rustdoc) - #161880 (fix rustc_lint_defs doctest issues) - #161883 (better deal with internal features being injected into doctests) - #161887 (std: uefi: fix File::seek returning the EOF sentinel) - #161897 (Reject contract attributes without arguments) - #161909 (Report the configured Polonius default in -Z help) - #161910 (Add rustdoc-html regression test for generated macro) - #161914 (Retroactively add relnotes for `bool::{ok_or,ok_or_else}` (1.98.0)) - #161924 (Windows: document that `normalize_lexically` converts `/` to `\`) - #161927 (Change `rustc_middle/src/hooks/mod.rs` to `hooks.rs`)

Rollup of 17 pull requests Successful merges: - #161945 (std: optimise IO error formatting) - #160594 (attach global target features to module-level assembly) - #161577 (implement [u8]::split_ascii_whitespace) - #161858 (fix ICE in generic_const_parameter_types with inherents) - #161377 ([bootstrap] Don't reverse the order of dylib search path entries) - #161804 (Document PartialOrd behavior for Option<T> where T: PartialOrd) - #161865 (loongarch: support passing `u128`/`i128` to inline assembly) - #161877 (Do not load macro metadata for local definitions in rustdoc) - #161880 (fix rustc_lint_defs doctest issues) - #161883 (better deal with internal features being injected into doctests) - #161887 (std: uefi: fix File::seek returning the EOF sentinel) - #161888 (compiler: Allow safestack to be togglable via #[sanitize(safestack = "...")]) - #161897 (Reject contract attributes without arguments) - #161910 (Add rustdoc-html regression test for generated macro) - #161914 (Retroactively add relnotes for `bool::{ok_or,ok_or_else}` (1.98.0)) - #161924 (Windows: document that `normalize_lexically` converts `/` to `\`) - #161927 (Change `rustc_middle/src/hooks/mod.rs` to `hooks.rs`)

@nikic

Rollup merge of #160594 - folkertdev:asm-forward-target-features, r=nikic attach global target features to module-level assembly fixes #80608 fixes #127269 At long last, we can forward global target features to LLVM and it will preserve the target features a block of module-level assembly was defined with through LTO. cc @nikic (who made this happen) cc @RalfJung any nasty side-effects we might be overlooking here?

tgross35 · 2026-09-17T01:36:13Z

@rustbot label +beta-nominated

There is another backport (#162820) needed to unbreak builds on thumb targets, which is currently relying on this PR in stable. There are possible workarounds but they're not as elegant as having this backported, it seems worth considering given the change is reasonably self-contained.

See some more context at #t-compiler/help > Valid `bx lr` rejected on arm.

apiraino · 2026-09-17T09:21:39Z

@folkertdev would you be in favor to backport this?

folkertdev · 2026-09-17T09:25:54Z

Yes, that seems fine to me. We've not seen any issues since merging this.

rustbot · 2026-09-17T10:08:37Z

beta backport approved as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels are handled by them.

apiraino · 2026-09-17T12:48:39Z

Backport of this patch is cherry-picked in #162820 (see comment), so removing the nomination here to avoid confusion

@rustbot label -beta-nominated -beta-accepted

[beta] backports - Re-export `core::fmt::NumBuffer` in `alloc` (and `std`) #161430 - Make `VaArgSafe` dyn-incompatible #162374 - Destabilize `VaArgSafe` #162909 - attach global target features to module-level assembly #160594 - (partial) compiler-builtins subtree update - #162816 r? me

rustbot added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Aug 5, 2026

rustbot assigned petrochenkov Aug 5, 2026

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 Aug 5, 2026

folkertdev commented Aug 5, 2026

View reviewed changes