Skip to content

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Sep 18, 2025

When this code was introduced in #130446 and #131805, it was complicated by the need to maintain compatibility with earlier versions of LLVM.

Now that LLVM 20 is the baseline (#145071), we can do all of the quoting in pure Rust code, and pass two flat strings to LLVM to be used as-is.


In this PR, my priority has been to preserve the existing behaviour as much as possible, without worrying too much about what the behaviour should be. (Though I did avoid a leading space before the first argument.)

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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 Sep 18, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 18, 2025

r? @lcnr

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

@Zalathar
Copy link
Contributor Author

cc @durin42 @nebulark @aeubanks (contributors to the C++ code being replaced)

@Zalathar
Copy link
Contributor Author

@bors try jobs=x86_64-msvc-1,x86_64-msvc-2

rust-bors bot added a commit that referenced this pull request Sep 18, 2025
cg_llvm: Move target machine command-line quoting from C++ to Rust

try-job: x86_64-msvc-1
try-job: x86_64-msvc-2
@rust-bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-bors
Copy link

rust-bors bot commented Sep 18, 2025

💔 Test for bee730a failed: CI. Failed jobs:

@Zalathar
Copy link
Contributor Author

Oh I see, setting Quote = true causes each argument to always be quoted, regardless of whether it contains any parts that would need quoting.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 18, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@Zalathar
Copy link
Contributor Author

I've heard second-hand that this function shows up on perf results. I don't know whether that's true, but it can't hurt to check.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Sep 18, 2025
cg_llvm: Move target machine command-line quoting from C++ to Rust
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 18, 2025
@rust-bors
Copy link

rust-bors bot commented Sep 18, 2025

☀️ Try build successful (CI)
Build commit: 44b341a (44b341a1c7f27c26b78205e3264d584b69a9c53f, parent: 93117677d857bb7c3f12c9dc500d77839f8fb13d)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (44b341a): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
0.2% [0.2%, 0.2%] 2
Regressions ❌
(secondary)
0.2% [0.1%, 0.3%] 16
Improvements ✅
(primary)
-0.4% [-0.5%, -0.2%] 4
Improvements ✅
(secondary)
-3.1% [-23.4%, -0.2%] 15
All ❌✅ (primary) -0.2% [-0.5%, 0.2%] 6

Max RSS (memory usage)

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

Cycles

Results (secondary -5.3%)

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.1% [2.1%, 2.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-9.0% [-9.4%, -8.6%] 2
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 471.663s -> 472.562s (0.19%)
Artifact size: 387.94 MiB -> 387.87 MiB (-0.02%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 18, 2025
@Zalathar
Copy link
Contributor Author

There's more followup that could be done here, like doing this quoting only once (instead of per-CGU), or skipping it for non-PDB targets. But I think that just moving the quoting step from C++ to Rust is worth landing on its own.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 19, 2025
@lcnr
Copy link
Contributor

lcnr commented Sep 19, 2025

r? @nikic (i can't read the replaced c++ code ✨ )

@rustbot rustbot assigned nikic and unassigned lcnr Sep 19, 2025
@nikic
Copy link
Contributor

nikic commented Sep 19, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 19, 2025

📌 Commit 8b0a254 has been approved by nikic

It is now in the queue for this repository.

@bors 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 Sep 19, 2025
@bors
Copy link
Collaborator

bors commented Sep 19, 2025

⌛ Testing commit 8b0a254 with merge 0be8e16...

@bors
Copy link
Collaborator

bors commented Sep 19, 2025

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 0be8e16 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 19, 2025
@bors bors merged commit 0be8e16 into rust-lang:master Sep 19, 2025
12 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Sep 19, 2025
Copy link
Contributor

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 5904356 (parent) -> 0be8e16 (this PR)

Test differences

Show 2 test diffs

2 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 0be8e16088894483a7012c5026c3247c14a0c3c2 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. aarch64-apple: 5671.4s -> 6397.4s (12.8%)
  2. dist-aarch64-apple: 6108.6s -> 6660.7s (9.0%)
  3. x86_64-mingw-2: 7795.4s -> 8429.4s (8.1%)
  4. dist-x86_64-windows-gnullvm: 4769.5s -> 5093.2s (6.8%)
  5. dist-x86_64-apple: 7808.8s -> 7298.5s (-6.5%)
  6. x86_64-gnu: 6956.3s -> 6581.8s (-5.4%)
  7. x86_64-msvc-2: 7157.6s -> 6782.6s (-5.2%)
  8. x86_64-msvc-ext3: 6312.6s -> 6627.0s (5.0%)
  9. dist-ohos-armv7: 4152.6s -> 3958.7s (-4.7%)
  10. dist-x86_64-mingw: 9413.0s -> 9046.7s (-3.9%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0be8e16): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.4% [-0.6%, -0.2%] 4
Improvements ✅
(secondary)
-3.0% [-23.4%, -0.2%] 16
All ❌✅ (primary) -0.4% [-0.6%, -0.2%] 4

Max RSS (memory usage)

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

Cycles

Results (secondary -4.8%)

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.5% [2.5%, 2.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-8.4% [-9.5%, -7.3%] 2
All ❌✅ (primary) - - 0

Binary size

Results (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
Regressions ❌
(secondary)
0.0% [0.0%, 0.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Bootstrap: 471.972s -> 472.625s (0.14%)
Artifact size: 389.96 MiB -> 389.93 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants