cg_llvm: Use fewer FFI calls to check the target CPU's features by Zalathar · Pull Request #163143 · rust-lang/rust

Zalathar · 2026-09-22T03:33:55Z

The existing code makes a separate FFI call to MCSubtargetInfo::checkFeatures for each feature-dependency of the feature being checked, and also performs string-manipulation on the C++ side to add a leading + to each feature name.

What we can do instead is prepare a single string on the Rust side in the form "+foo,+bar,+baz," for each Rust target feature, and pass that to LLVM as a pointer/length string. LLVM already knows how to check that all of the listed features are present.

This change makes the code simpler overall. All of the string manipulation now takes place either in Rust code or in LLVM itself, and not in our C++ wrapper.

There should be no change to compiler output.

Zalathar · 2026-09-22T03:34:14Z

@bors try @rust-timer queue

cg_llvm: Use fewer FFI calls to check the target CPU's features

rust-bors · 2026-09-22T04:32:31Z

☀️ Try build successful (CI)
Build commit: e047fe3 (e047fe3142fe7f4c66385c976f3c4b7b1e5b8b15)
Base parent: 88638df (88638df1e741778019e42fcdf307072865d19501)

rust-timer · 2026-09-22T05:16:40Z

Finished benchmarking commit (e047fe3): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (secondary 2.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 2.8%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.8%	[2.8%, 2.8%]	1
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	2.8%	[2.8%, 2.8%]	1

Binary size

Results (primary 0.0%, secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.1%]	11
Regressions ❌ (secondary)	0.0%	[0.0%, 0.1%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.1%]	11

Bootstrap: 487.826s -> 488.235s (0.08%)
Artifact size: 406.29 MiB -> 406.39 MiB (0.03%)

Zalathar · 2026-09-22T05:21:04Z

No measurable perf effect, but I think this still makes sense on its own, as the Rust-side and C++-side code are both simpler.

Zalathar · 2026-09-22T05:37:26Z

-                    // `has_feature` is moderately expensive. On targets with many
-                    // features (e.g. x86) these calls take a non-trivial fraction of runtime
-                    // when compiling very small programs.


I believe this comment was made obsolete by llvm/llvm-project#130936, which is included in LLVM 21.

View changes since the review

rustbot · 2026-09-22T05:48:21Z

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

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler
compiler expanded to 77 candidates
Random selection from 19 candidates

davidtwco

r=me after checking comment

View changes since this review

davidtwco · 2026-09-23T15:55:57Z

+    pub(crate) fn has_features(&self, features: llvm_util::LLVMFeature<'_>) -> bool {
+        // Convert to a feature string expected by LLVM's `SubtargetFeatures`.
+        // All of the required LLVM features must be enabled.
+        let features = features.into_iter().flat_map(|feat| ["+", feat, ","]).collect::<String>();


I assume there's no issues with the trailing comma here at the end?

Yeah, the split-on-commas in LLVM's SubtargetFeatures::Split sets KeepEmpty=false, so the empty segment after the trailing comma is safely discarded.

I added a comment to mention this.

The existing code makes a separate FFI call to `MCSubtargetInfo::checkFeatures` for each feature-dependency of the feature being checked, and also performs string-manipulation on the C++ side to add a leading '+' to each feature name. What we can do instead is prepare a single string on the Rust side in the form `"+foo,+bar,+baz,"` for each Rust target feature, and pass that string to LLVM. LLVM already knows how to check that all of the listed features are present. (The trailing comma is not a problem, because `SubtargetFeatures::Split` will automatically discard empty substrings after splitting on commas.)

rustbot · 2026-09-24T01:00:20Z

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.

Zalathar · 2026-09-24T02:44:11Z

Added a comment about the trailing comma being OK.

@bors r=davidtwco

rust-bors · 2026-09-24T02:44:14Z

📌 Commit 733daee has been approved by davidtwco

It is now in the queue for this repository.

…uwer Rollup of 14 pull requests Successful merges: - #162976 (fix quadratic naming of duplicate sidebar links) - #161275 (Refactor `core::cmp::{smallest, largest}` & add `mir-opt` test) - #163143 (cg_llvm: Use fewer FFI calls to check the target CPU's features) - #163188 (Adjust for Arm64EC name mangling when checking for exported symbols) - #163211 (`rustc_builtin_macros` cleanup, part 6) - #161386 (Don't merge distinct impl candidates) - #162942 (Remove `StashKey::AssociatedTypeSuggestion`) - #163096 (Don't suggest `std::` rustfix paths in `#![no_std]` crates) - #163110 (Mark `std::os::wasip2` with correct doc-cfgs, mark as unstable) - #163185 (properly decrement available_depth on cycles and provisional cache hits) - #163214 (revert r14 register names for arm) - #163226 (miri subtree update) - #163228 (Add regression test for trait predicate with escaping bounds) - #163234 (`rustc_dump_symbol_name`: add demangling information as a note instead)

Rollup merge of #163143 - Zalathar:check-features, r=davidtwco cg_llvm: Use fewer FFI calls to check the target CPU's features The existing code makes a separate FFI call to `MCSubtargetInfo::checkFeatures` for each feature-dependency of the feature being checked, and also performs string-manipulation on the C++ side to add a leading `+` to each feature name. What we can do instead is prepare a single string on the Rust side in the form `"+foo,+bar,+baz,"` for each Rust target feature, and pass that to LLVM as a pointer/length string. LLVM already knows how to check that all of the listed features are present. This change makes the code simpler overall. All of the string manipulation now takes place either in Rust code or in LLVM itself, and not in our C++ wrapper. There should be no change to compiler output.

…uwer Rollup of 14 pull requests Successful merges: - rust-lang/rust#162976 (fix quadratic naming of duplicate sidebar links) - rust-lang/rust#161275 (Refactor `core::cmp::{smallest, largest}` & add `mir-opt` test) - rust-lang/rust#163143 (cg_llvm: Use fewer FFI calls to check the target CPU's features) - rust-lang/rust#163188 (Adjust for Arm64EC name mangling when checking for exported symbols) - rust-lang/rust#163211 (`rustc_builtin_macros` cleanup, part 6) - rust-lang/rust#161386 (Don't merge distinct impl candidates) - rust-lang/rust#162942 (Remove `StashKey::AssociatedTypeSuggestion`) - rust-lang/rust#163096 (Don't suggest `std::` rustfix paths in `#![no_std]` crates) - rust-lang/rust#163110 (Mark `std::os::wasip2` with correct doc-cfgs, mark as unstable) - rust-lang/rust#163185 (properly decrement available_depth on cycles and provisional cache hits) - rust-lang/rust#163214 (revert r14 register names for arm) - rust-lang/rust#163226 (miri subtree update) - rust-lang/rust#163228 (Add regression test for trait predicate with escaping bounds) - rust-lang/rust#163234 (`rustc_dump_symbol_name`: add demangling information as a note instead)

panstromek · 2026-09-29T05:58:26Z

perf triage:

Looks like this caused a small compile time regression for CTFE stress test in #163244. This can also be just noise (well, it definitely is at least partially), but some part of the increase persisted. Does that make sense for this PR? Do we know where this could come from?

Zalathar · 2026-09-29T07:22:28Z

This PR had an earlier perf run at #163143 (comment) that gave sub-threshold improvement in ctfe-stress, so it would be weird for that to suddenly turn into a measurable regression.

In terms of the actual changes, it would also be surprising for this PR to only affect one benchmark, as I believe it’s touching code that only runs once per session and doesn’t scale with input size.

panstromek · 2026-09-29T07:51:48Z

Yea, this is probably just noise. As I went through the rest of the PRs in the rollup, I encountered the same regression again on ~4 of them.

This comment has been minimized.

Sign in to view

rust-bors Bot pushed a commit that referenced this pull request Sep 22, 2026

Auto merge of #163143 - Zalathar:check-features, r=<try>

e047fe3

cg_llvm: Use fewer FFI calls to check the target CPU's features

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 22, 2026

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 22, 2026

Zalathar force-pushed the check-features branch from 811b1bd to acd485c Compare September 22, 2026 05:35

Zalathar commented Sep 22, 2026

View reviewed changes

Zalathar marked this pull request as ready for review September 22, 2026 05:48

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 22, 2026

rustbot assigned davidtwco Sep 22, 2026

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 22, 2026

Zalathar force-pushed the check-features branch from acd485c to bdc4b77 Compare September 22, 2026 05:49

davidtwco approved these changes Sep 23, 2026

View reviewed changes

Zalathar force-pushed the check-features branch from bdc4b77 to 733daee Compare September 24, 2026 01:00

rust-bors Bot 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 Sep 24, 2026

JonathanBrouwer mentioned this pull request Sep 24, 2026

Rollup of 14 pull requests #163244

Merged

rust-bors Bot merged commit 5c845c6 into rust-lang:main Sep 24, 2026
13 checks passed

rustbot added this to the 1.100.0 milestone Sep 24, 2026

Zalathar deleted the check-features branch September 24, 2026 23:25

Uh oh!

Conversation

Zalathar commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zalathar commented Sep 22, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors Bot commented Sep 22, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Sep 22, 2026

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Zalathar commented Sep 22, 2026

Uh oh!

Zalathar Sep 22, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Sep 22, 2026

Uh oh!

davidtwco left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidtwco Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

Zalathar Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

rustbot commented Sep 24, 2026

Uh oh!

Zalathar commented Sep 24, 2026

Uh oh!

rust-bors Bot commented Sep 24, 2026

Uh oh!

Uh oh!

panstromek commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zalathar commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panstromek commented Sep 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Zalathar commented Sep 22, 2026 •

edited

Loading

Zalathar Sep 22, 2026 •

edited by rustbot

Loading

davidtwco left a comment •

edited by rustbot

Loading

panstromek commented Sep 29, 2026 •

edited

Loading

Zalathar commented Sep 29, 2026 •

edited

Loading