Port most of `--print=target-cpus` to Rust #132514

Zalathar · 2024-11-02T11:25:30Z

The logic and formatting needed by --print=target-cpus has historically been carried out in C++ code. Originally it used printf to write directly to the console, but later it switched over to writing to a std::ostringstream and then passing its buffer to a callback function pointer.

This PR replaces that C++ code with a very simple function that writes a list of CPU names to a &RustString, with the rest of the logic and formatting being handled by ordinary safe Rust code.

rustbot · 2024-11-02T11:25:38Z

r? @nnethercote

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

rustbot · 2024-11-02T11:25:41Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

Zalathar · 2024-11-02T11:44:54Z

There is only one existing test for this flag (tests/ui/codegen/target-cpus.rs), and that test doesn't cover the special handling of "native", so I've written a new test aimed specifically at that.

Adding snapshot-based tests for this seems like it would be a maintenance nightmare, so I wrote it as a custom run-make test that only checks the start of the output, and ignores the specific host CPU name.

jieyouxu · 2024-11-02T11:50:24Z

tests/run-make/print-target-cpus-native/rmake.rs

+    // Now try some cross-target queries with the same arch as the host.
+    // (Specify multiple targets so that at least one of them is not the host.)
+    let cross_targets: &[&str] = if cfg!(target_arch = "aarch64") {
+        &["aarch64-unknown-linux-gnu", "aarch64-apple-darwin"]


Question: ... would this need needs-llvm-component

Hmm, I'm not sure.

My guess is no, because it's guaranteed by ignore-cross-compile that we at least have the relevant arch component for the host system, and we're only testing other targets with the same arch as the host.

But I know very little about LLVM components.

I asked because I have dealt with run-make tests that seemed innocent (like see #129390). Apparently even --print=target-list needed relevant llvm components to be available.

So I think we should have the //@ needs-llvm-components for the cross-compiled targets: usually we have them (e.g. CI LLVM or locally), so it would be trivially satisfied.

If the condition will usually be true in CI and locally, then I don't mind adding the directive as a precaution.

Yeah, it's mostly just that some contributors do build local llvms without unneeded components.

jieyouxu

Thanks, this LGTM, feel free to r=me after PR CI is green.

Zalathar · 2024-11-02T14:06:53Z

🟩

@bors r=jieyouxu

bors · 2024-11-02T14:06:55Z

📌 Commit 90f2075 has been approved by jieyouxu

It is now in the queue for this repository.

…ouxu Port most of `--print=target-cpus` to Rust The logic and formatting needed by `--print=target-cpus` has historically been carried out in C++ code. Originally it used `printf` to write directly to the console, but later it switched over to writing to a `std::ostringstream` and then passing its buffer to a callback function pointer. This PR replaces that C++ code with a very simple function that writes a list of CPU names to a `&RustString`, with the rest of the logic and formatting being handled by ordinary safe Rust code.

…kingjubilee Rollup of 12 pull requests Successful merges: - rust-lang#132393 (Docs: added brief colon explanation) - rust-lang#132419 (PassWrapper: adapt for llvm/llvm-project@b01e2a8b5620466c3b) - rust-lang#132437 (coverage: Regression test for inlining into an uninstrumented crate) - rust-lang#132458 (get rid of a whole bunch of unnecessary rustc_const_unstable attributes) - rust-lang#132499 (unicode_data.rs: show command for generating file) - rust-lang#132503 (better test for const HashMap; remove const_hash leftovers) - rust-lang#132514 (Port most of `--print=target-cpus` to Rust) - rust-lang#132520 (NFC add known bug nr to test) - rust-lang#132522 (make codegen help output more consistent) - rust-lang#132523 (Added regression test for generics index out of bounds) - rust-lang#132526 (Subtree sync for rustc_codegen_cranelift) - rust-lang#132528 (Use `*_opt` typeck results fns to not ICE in fallback suggestion) - rust-lang#132540 (Do not format generic consts) Failed merges: - rust-lang#132511 (stabilize const_arguments_as_str) r? `@ghost` `@rustbot` modify labels: rollup

workingjubilee · 2024-11-03T02:44:19Z

this PR combined with the LLVM fail in #132546 which ought to be spurious is giving me a very weird vibe, so this is getting marked iffy out of an abundance of caution.

@bors rollup=iffy

bors · 2024-11-03T11:09:41Z

⌛ Testing commit 90f2075 with merge 59ae5eb...

bors · 2024-11-03T13:45:40Z

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 59ae5eb to master...

rust-timer · 2024-11-03T15:04:17Z

Finished benchmarking commit (59ae5eb): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

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

Max RSS (memory usage)

Results (primary 4.4%, secondary 0.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

Results (secondary 2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 779.49s -> 779.022s (-0.06%)
Artifact size: 335.37 MiB -> 335.35 MiB (-0.00%)

rustbot assigned nnethercote Nov 2, 2024

rustbot added A-run-make Area: port run-make Makefiles to rmake.rs 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 Nov 2, 2024

Zalathar force-pushed the print-target-cpus branch 2 times, most recently from 64d4573 to 1320d7b Compare November 2, 2024 11:40

jieyouxu reviewed Nov 2, 2024

View reviewed changes

Zalathar added 3 commits November 2, 2024 23:39

Add a run-make test for same-arch --print=target-cpus

36040aa

Use a dedicated safe wrapper for LLVMRustGetHostCPUName

0fa86f9

Port most of LLVMRustPrintTargetCPUs to Rust

90f2075

Zalathar force-pushed the print-target-cpus branch from 1320d7b to 90f2075 Compare November 2, 2024 12:39

jieyouxu approved these changes Nov 2, 2024

View reviewed changes

jieyouxu assigned jieyouxu and unassigned nnethercote Nov 2, 2024

bors 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 Nov 2, 2024

workingjubilee mentioned this pull request Nov 2, 2024

Rollup of 12 pull requests #132546

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 3, 2024

bors merged commit 59ae5eb into rust-lang:master Nov 3, 2024
7 checks passed

rustbot added this to the 1.84.0 milestone Nov 3, 2024

Zalathar deleted the print-target-cpus branch November 3, 2024 13:49

Zalathar mentioned this pull request Nov 3, 2024

Make RustString an extern type to avoid improper_ctypes warnings #132549

Merged

Uh oh!

Port most of --print=target-cpus to Rust #132514

Port most of --print=target-cpus to Rust #132514

Uh oh!

Conversation

Zalathar commented Nov 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Nov 2, 2024

Uh oh!

rustbot commented Nov 2, 2024

Uh oh!

Zalathar commented Nov 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jieyouxu Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

Zalathar Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

jieyouxu Nov 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zalathar Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

jieyouxu Nov 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu left a comment

Choose a reason for hiding this comment

Uh oh!

Zalathar commented Nov 2, 2024

Uh oh!

bors commented Nov 2, 2024

Uh oh!

workingjubilee commented Nov 3, 2024

Uh oh!

bors commented Nov 3, 2024

Uh oh!

bors commented Nov 3, 2024

Uh oh!

Uh oh!

rust-timer commented Nov 3, 2024

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Port most of `--print=target-cpus` to Rust #132514

Port most of `--print=target-cpus` to Rust #132514

Zalathar commented Nov 2, 2024 •

edited

Loading

Zalathar commented Nov 2, 2024 •

edited

Loading

jieyouxu Nov 2, 2024 •

edited

Loading

jieyouxu Nov 2, 2024 •

edited

Loading