Use `preserve_mostcc` for `extern "rust-cold"` #115260

scottmcm · 2023-08-27T00:44:20Z

As experimentation in #115242 has shown looks better than coldcc. Notably, clang exposes preserve_most (https://clang.llvm.org/docs/AttributeReference.html#preserve-most) but not cold, so this change should put us on a better-supported path.

And don't use a different convention for cold on Windows, because that actually ends up making things worse. (See comment in the code.)

cc tracking issue #97544

As experimentation in 115242 has shown looks better than `coldcc`. And *don't* use a different convention for cold on Windows, because that actually ends up making things worse. cc tracking issue 97544

rustbot · 2023-08-27T00:44:25Z

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-08-27T01:49:09Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

These commits modify compiler targets.
(See the Target Tier Policy.)

bjorn3 · 2023-08-27T07:10:27Z

compiler/rustc_codegen_cranelift/src/abi/mod.rs

@@ -39,7 +39,7 @@ fn clif_sig_from_fn_abi<'tcx>(
 pub(crate) fn conv_to_call_conv(sess: &Session, c: Conv, default_call_conv: CallConv) -> CallConv {
    match c {
        Conv::Rust | Conv::C => default_call_conv,
-        Conv::RustCold => CallConv::Cold,
+        Conv::Cold | Conv::PreserveMost | Conv::PreserveAll => CallConv::Cold,


None of these are ABI compatible between LLVM and Cranelift actually. So using them in the standard library will break ABI compatibility. It should be possible to implement in Cranelift if LLVM guarantees a specific call conv for them, but that doesn't help for GCC.

Hmm, I guess that's not a blocker for this PR, though, since it was coldcc before, which if I understand correctly already wasn't ABI compatible.

What's your meta expectation here for the future? Do all externs have to be interoperable between backends? That would seem a shame, as it would essentially force extern "Rust" to always be the same as extern "C", rather than letting a backend innovate something faster. Would it be ok to have options to force rust-cold to be something else (maybe just C) for people who need the interop?

Do all externs have to be interoperable between backends?

Preferably, but if any is not interoperable, denying usage in the standard library and emitting a hard error if caller and callee are compiled with a different codegen backend would work too I guess. (eg by internally lowering it to a different call conv depending on the selected codegen backend before we do typeck)

That would seem a shame, as it would essentially force extern "Rust" to always be the same as extern "C", rather than letting a backend innovate something faster.

extern "Rust" could be changed to internally use eg extern "sysv" on all targets or extern "win64" on all x86_64 targets. Note that extern "Rust" is already not extern "C". The lowering from rust types to primitives is different, but those primitives are passed in the same way such that it is actually possible for all backends to use the same ABI by reusing the lowering code in rustc_target. I don't want to change that.

Would it be ok to have options to force rust-cold to be something else (maybe just C) for people who need the interop?

That wouldn't help if the standard library uses it as the LLVM compiled standard library will be used with cg_clif once it ships on nightly.

WaffleLapkin

The changes look good to me. It looks like cranelift/llvm ABI mismatch is an existing issue and should be addressed separately (could someone make sure we track this somewhere?), if this is indeed the case — r=me.

scottmcm · 2023-08-28T05:46:59Z

Yes, I think this PR is still good progress -- thinks like moving the abi-vs-conv translation point to a better place, for example -- and since it's not used anywhere, it's ok if it's not quite usable in general just yet.

I've added an Unresolved Question to the tracking issue for extern "rust-cold" linking back to bjorn3's comments above.

@bors r=WaffleLapkin

bors · 2023-08-28T05:47:01Z

📌 Commit 754f488 has been approved by WaffleLapkin

It is now in the queue for this repository.

bors · 2023-08-28T12:51:15Z

⌛ Testing commit 754f488 with merge a069e3f9c84eaff5d70655447602974f15124d27...

bors · 2023-08-28T13:00:03Z

💔 Test failed - checks-actions

rust-log-analyzer · 2023-08-28T13:01:28Z

The job dist-x86_64-msvc failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    0: error decoding response body: operation timed out
    1: operation timed out

Stack backtrace:
   0: backtrace::backtrace::dbghelp::trace
             at C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\backtrace-0.3.69\src\backtrace\dbghelp.rs:98
             at C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\backtrace-0.3.69\src\backtrace\mod.rs:66
             at C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\backtrace-0.3.69\src\backtrace\mod.rs:66
   2: backtrace::backtrace::trace<anyhow::backtrace::capture::impl$4::create::closure_env$0>
             at C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\backtrace-0.3.69\src\backtrace\mod.rs:53
   3: anyhow::backtrace::capture::Backtrace::create
             at C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\anyhow-1.0.75\src\backtrace.rs:216
   4: anyhow::backtrace::capture::Backtrace::capture
             at C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\anyhow-1.0.75\src\backtrace.rs:204
   5: anyhow::error::impl$1::from<reqwest::error::Error>
             at C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\anyhow-1.0.75\src\error.rs:551
   6: opt_dist::environment::windows::impl$1::prepare_rustc_perf
             at /rustc/a069e3f9c84eaff5d70655447602974f15124d27/src\tools\opt-dist\src\environment\windows.rs:46
   7: opt_dist::execute_pipeline::closure$0
             at /rustc/a069e3f9c84eaff5d70655447602974f15124d27/src\tools\opt-dist\src\main.rs:37
   8: opt_dist::utils::with_log_group<opt_dist::execute_pipeline::closure_env$0,enum2$<core::result::Result<tuple$<>,anyhow::Error> > >
             at /rustc/a069e3f9c84eaff5d70655447602974f15124d27/src\tools\opt-dist\src\utils\mod.rs:65
             at /rustc/a069e3f9c84eaff5d70655447602974f15124d27/src\tools\opt-dist\src\main.rs:207
  10: core::ops::function::FnOnce::call_once
             at /rustc/680cdf8168a906b4ea80af673c64e4a16f77be57\library\core\src\ops\function.rs:250
             at /rustc/680cdf8168a906b4ea80af673c64e4a16f77be57\library\core\src\ops\function.rs:250
  11: std::sys_common::backtrace::__rust_begin_short_backtrace<enum2$<core::result::Result<tuple$<>,anyhow::Error> > (*)(),enum2$<core::result::Result<tuple$<>,anyhow::Error> > >
             at /rustc/680cdf8168a906b4ea80af673c64e4a16f77be57\library\std\src\sys_common\backtrace.rs:154
             at /rustc/680cdf8168a906b4ea80af673c64e4a16f77be57\library\std\src\rt.rs:166
  13: core::ops::function::FnOnce::call_once
             at /rustc/680cdf8168a906b4ea80af673c64e4a16f77be57\library\core\src\ops\function.rs:250
             at /rustc/680cdf8168a906b4ea80af673c64e4a16f77be57\library\core\src\ops\function.rs:250
  14: core::ops::function::FnOnce::call_once<std::rt::lang_start::closure_env$0<enum2$<core::result::Result<tuple$<>,anyhow::Error> > >,tuple$<> >
             at /rustc/680cdf8168a906b4ea80af673c64e4a16f77be57\library\std\src\rt.rs:166
             at /rustc/680cdf8168a906b4ea80af673c64e4a16f77be57/library\std\src\rt.rs:148
  16: std::panicking::try::do_call
             at /rustc/680cdf8168a906b4ea80af673c64e4a16f77be57/library\std\src\panicking.rs:524
  17: std::panicking::try

scottmcm · 2023-08-28T17:03:31Z

@bors retry

bors · 2023-08-28T22:49:35Z

⌛ Testing commit 754f488 with merge 59c2f8a62fef237eb5335173439fecf6a06e8793...

bors · 2023-08-28T22:56:03Z

💔 Test failed - checks-actions

rust-log-analyzer · 2023-08-28T22:56:45Z

The job dist-apple-various failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

SCRIPT=./x.py dist bootstrap --include-default-paths --host='' --target=aarch64-apple-ios,x86_64-apple-ios,aarch64-apple-ios-sim
SELECT_XCODE=/Applications/Xcode_13.4.1.app
SHELL=/bin/bash
SHLVL=3
SSH_AUTH_SOCK=/private/tmp/com.apple.launchd.zCw5ctzj4q/Listeners
STATS_EXTP=https://provjobdsettingscdn.blob.core.windows.net/settings/provjobdsettings-0.5.154/provjobd.data
STATS_RDCL=true
STATS_TIS=mining
STATS_TRP=true

scottmcm · 2023-08-29T00:49:34Z

@bors retry (curl: (6) Could not resolve host: static.rust-lang.org)

bors · 2023-08-29T02:23:46Z

⌛ Testing commit 754f488 with merge f3284dc...

bors · 2023-08-29T04:11:04Z

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing f3284dc to master...

rust-timer · 2023-08-29T08:21:25Z

Finished benchmarking commit (f3284dc): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

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

Cycles

Results

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

Binary size

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

Bootstrap: 630.211s -> 631.106s (0.14%)
Artifact size: 316.20 MiB -> 316.27 MiB (0.02%)

Use preserve_mostcc for extern "rust-cold"

754f488

As experimentation in 115242 has shown looks better than `coldcc`. And *don't* use a different convention for cold on Windows, because that actually ends up making things worse. cc tracking issue 97544

rustbot assigned petrochenkov Aug 27, 2023

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 27, 2023

scottmcm mentioned this pull request Aug 27, 2023

Tracking Issue for extern "rust-cold" ABI (feature rust_cold_cc) #97544

Open

8 tasks

scottmcm marked this pull request as ready for review August 27, 2023 01:49

scottmcm mentioned this pull request Aug 27, 2023

Vec::push should load a passed-by-pointer argument *after* reserve_for_push #115242

Open

bjorn3 reviewed Aug 27, 2023

View reviewed changes

WaffleLapkin approved these changes Aug 27, 2023

View reviewed changes

WaffleLapkin assigned WaffleLapkin and unassigned petrochenkov Aug 27, 2023

WaffleLapkin 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 Aug 27, 2023

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 28, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 28, 2023

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 Aug 28, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 28, 2023

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 Aug 29, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 29, 2023

bors merged commit f3284dc into rust-lang:master Aug 29, 2023
11 checks passed

rustbot added this to the 1.74.0 milestone Aug 29, 2023

matthiaskrgr mentioned this pull request Mar 27, 2024

univariant: field #2 comes after unsized field #123134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `preserve_mostcc` for `extern "rust-cold"` #115260

Use `preserve_mostcc` for `extern "rust-cold"` #115260

scottmcm commented Aug 27, 2023 •

edited

Loading

rustbot commented Aug 27, 2023

rustbot commented Aug 27, 2023

bjorn3 Aug 27, 2023

scottmcm Aug 27, 2023

bjorn3 Aug 27, 2023

WaffleLapkin left a comment

scottmcm commented Aug 28, 2023

bors commented Aug 28, 2023

bors commented Aug 28, 2023

bors commented Aug 28, 2023

rust-log-analyzer commented Aug 28, 2023

scottmcm commented Aug 28, 2023

bors commented Aug 28, 2023

bors commented Aug 28, 2023

rust-log-analyzer commented Aug 28, 2023

scottmcm commented Aug 29, 2023

bors commented Aug 29, 2023

bors commented Aug 29, 2023

rust-timer commented Aug 29, 2023

Use preserve_mostcc for extern "rust-cold" #115260

Use preserve_mostcc for extern "rust-cold" #115260

Conversation

scottmcm commented Aug 27, 2023 • edited Loading

rustbot commented Aug 27, 2023

rustbot commented Aug 27, 2023

bjorn3 Aug 27, 2023

Choose a reason for hiding this comment

scottmcm Aug 27, 2023

Choose a reason for hiding this comment

bjorn3 Aug 27, 2023

Choose a reason for hiding this comment

WaffleLapkin left a comment

Choose a reason for hiding this comment

scottmcm commented Aug 28, 2023

bors commented Aug 28, 2023

bors commented Aug 28, 2023

bors commented Aug 28, 2023

rust-log-analyzer commented Aug 28, 2023

scottmcm commented Aug 28, 2023

bors commented Aug 28, 2023

bors commented Aug 28, 2023

rust-log-analyzer commented Aug 28, 2023

scottmcm commented Aug 29, 2023

bors commented Aug 29, 2023

bors commented Aug 29, 2023

rust-timer commented Aug 29, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Use `preserve_mostcc` for `extern "rust-cold"` #115260

Use `preserve_mostcc` for `extern "rust-cold"` #115260

scottmcm commented Aug 27, 2023 •

edited

Loading