Use the same mir-opt bless targets on all platforms #120060

saethlin · 2024-01-17T15:26:20Z

This undoes some of the implementation in #119035, but not the effect. Sorry for the churn, I've learned a lot about how all this works over the past few weeks.

The objective here is to make x test mir-opt --bless use the same set of targets on all platforms. It didn't do that from the start because bootstrap assumes that a target linker is available, so the availability of cross-linkers is how we ended up with MIR_OPT_BLESS_TARGET_MAPPING and poor support for blessing mir-opt tests from Aarch64 MacOS. This PR corrects that.

So I've adjusted the bless targets for mir-opt tests, as well as tweaked some of the logic in bootstrap about linker configuration so that we don't try to access the cache of cc/linker configuration when doing the mir-opt builds.

While working on that I realized that if I swapped from the cargo rustc -p std strategy to cargo check on the sysroot, I could use the existing code for check builds to bypass some linker logic. Sweet.

But just doing that doesn't work, because then mir-opt tests complain that they can't find an rlib for any of the standard library crates. That happens because nearly all the mir-opt tests are attempting to build CrateType::Executable. We already have all the MIR required for mir-opt tests from the rmeta files, but since rustc think we're trying to build an executable it demands we have access to all the upstream monomorphizations that only exist in rlibs, not the meta files in a MIR-only sysroot.

So to fix that, I've swapped all the mir-opt tests be passed --crate-type=rlib. That works, but leaves us with a few broken mir-opt tests which I've blessed or fixed up; we also lose MIR for some functions so I added -Clink-dead-code to paper over that. The inlining changes are because changing the crate-type perturbs the hashes that are compared here to sometimes let us do inlining even in a possibly-recursive call:

rust/compiler/rustc_mir_transform/src/inline.rs

Lines 332 to 341 in 4cb17b4

    
           if callee_def_id.is_local() { 
        
               // Avoid a cycle here by only using `instance_mir` only if we have 
        
               // a lower `DefPathHash` than the callee. This ensures that the callee will 
        
               // not inline us. This trick even works with incremental compilation, 
        
               // since `DefPathHash` is stable. 
        
               if self.tcx.def_path_hash(caller_def_id).local_hash() 
        
                   < self.tcx.def_path_hash(callee_def_id).local_hash() 
        
               { 
        
                   return Ok(()); 
        
               }

rustbot · 2024-01-17T15:26:28Z

r? @clubby789

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

saethlin · 2024-01-21T18:52:54Z

cc @rust-lang/wg-mir-opt

src/bootstrap/src/core/build_steps/test.rs

src/tools/compiletest/src/runtest.rs

wesleywiser · 2024-01-30T22:58:38Z

r? wesleywiser

wesleywiser · 2024-01-31T15:49:26Z

This is awesome! Thanks for working on this @saethlin 👍

@bors r+

bors · 2024-01-31T15:49:29Z

📌 Commit 815d312 has been approved by wesleywiser

It is now in the queue for this repository.

…wesleywiser Use the same mir-opt bless targets on all platforms This undoes some of the implementation in rust-lang#119035, but not the effect. Sorry for the churn, I've learned a lot about how all this works over the past few weeks. The objective here is to make `x test mir-opt --bless` use the same set of targets on all platforms. It didn't do that from the start because bootstrap assumes that a target linker is available, so the availability of cross-linkers is how we ended up with `MIR_OPT_BLESS_TARGET_MAPPING` and poor support for blessing mir-opt tests from Aarch64 MacOS. This PR corrects that. So I've adjusted the bless targets for mir-opt tests, as well as tweaked some of the logic in bootstrap about linker configuration so that we don't try to access the cache of cc/linker configuration when doing the mir-opt builds. While working on that I realized that if I swapped from the `cargo rustc -p std` strategy to `cargo check` on the sysroot, I could use the existing code for check builds to bypass some linker logic. Sweet. But just doing that doesn't work, because then mir-opt tests complain that they can't find an rlib for any of the standard library crates. That happens because nearly all the mir-opt tests are attempting to build `CrateType::Executable`. We already have all the MIR required for mir-opt tests from the rmeta files, but since rustc think we're trying to build an executable it demands we have access to all the upstream monomorphizations that only exist in rlibs, not the meta files in a MIR-only sysroot. So to fix that, I've swapped all the mir-opt tests be passed `--crate-type=rlib`. That works, but leaves us with a few broken mir-opt tests which I've blessed or fixed up; we also lose MIR for some functions so I added `-Clink-dead-code` to paper over that. The inlining changes are because changing the crate-type perturbs the hashes that are compared here to sometimes let us do inlining even in a possibly-recursive call: https://github.com/rust-lang/rust/blob/4cb17b4e78e0540e49d2da884cc621a6bf6f47fa/compiler/rustc_mir_transform/src/inline.rs#L332-L341

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#116284 (make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern) - rust-lang#120060 (Use the same mir-opt bless targets on all platforms) - rust-lang#120214 (match lowering: consistently lower bindings deepest-first) - rust-lang#120326 (Actually abort in -Zpanic-abort-tests) - rust-lang#120396 (Account for unbounded type param receiver in suggestions) - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg) r? `@ghost` `@rustbot` modify labels: rollup

…wesleywiser Use the same mir-opt bless targets on all platforms This undoes some of the implementation in rust-lang#119035, but not the effect. Sorry for the churn, I've learned a lot about how all this works over the past few weeks. The objective here is to make `x test mir-opt --bless` use the same set of targets on all platforms. It didn't do that from the start because bootstrap assumes that a target linker is available, so the availability of cross-linkers is how we ended up with `MIR_OPT_BLESS_TARGET_MAPPING` and poor support for blessing mir-opt tests from Aarch64 MacOS. This PR corrects that. So I've adjusted the bless targets for mir-opt tests, as well as tweaked some of the logic in bootstrap about linker configuration so that we don't try to access the cache of cc/linker configuration when doing the mir-opt builds. While working on that I realized that if I swapped from the `cargo rustc -p std` strategy to `cargo check` on the sysroot, I could use the existing code for check builds to bypass some linker logic. Sweet. But just doing that doesn't work, because then mir-opt tests complain that they can't find an rlib for any of the standard library crates. That happens because nearly all the mir-opt tests are attempting to build `CrateType::Executable`. We already have all the MIR required for mir-opt tests from the rmeta files, but since rustc think we're trying to build an executable it demands we have access to all the upstream monomorphizations that only exist in rlibs, not the meta files in a MIR-only sysroot. So to fix that, I've swapped all the mir-opt tests be passed `--crate-type=rlib`. That works, but leaves us with a few broken mir-opt tests which I've blessed or fixed up; we also lose MIR for some functions so I added `-Clink-dead-code` to paper over that. The inlining changes are because changing the crate-type perturbs the hashes that are compared here to sometimes let us do inlining even in a possibly-recursive call: https://github.com/rust-lang/rust/blob/4cb17b4e78e0540e49d2da884cc621a6bf6f47fa/compiler/rustc_mir_transform/src/inline.rs#L332-L341

Zalathar · 2024-02-05T05:52:01Z

This appears to also fix some mysterious failures I encountered when testing mir-opt and codegen in the same command.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#113833 (`std::error::Error` -> Trait Implementations: lifetimes consistency improvement) - rust-lang#115386 (PartialEq, PartialOrd: update and synchronize handling of transitive chains) - rust-lang#116284 (make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern) - rust-lang#118960 (Add LocalWaker and ContextBuilder types to core, and LocalWake trait to alloc.) - rust-lang#120060 (Use the same mir-opt bless targets on all platforms) - rust-lang#120214 (match lowering: consistently lower bindings deepest-first) - rust-lang#120384 (Use `<T, U>` for array/slice equality `impl`s) r? `@ghost` `@rustbot` modify labels: rollup

saethlin · 2024-02-07T04:43:11Z

bors failed because a new test was added that assumed fn main would have MIR generated. An obscure form of merge conflict.

saethlin · 2024-02-07T04:55:34Z

Because this type of change has already been approved:
@bors r=wesleywiser

bors · 2024-02-07T04:55:36Z

📌 Commit 3c7a8b9 has been approved by wesleywiser

It is now in the queue for this repository.

bors · 2024-02-07T05:35:01Z

⌛ Testing commit 3c7a8b9 with merge 53061d3...

…sleywiser Use the same mir-opt bless targets on all platforms This undoes some of the implementation in rust-lang#119035, but not the effect. Sorry for the churn, I've learned a lot about how all this works over the past few weeks. The objective here is to make `x test mir-opt --bless` use the same set of targets on all platforms. It didn't do that from the start because bootstrap assumes that a target linker is available, so the availability of cross-linkers is how we ended up with `MIR_OPT_BLESS_TARGET_MAPPING` and poor support for blessing mir-opt tests from Aarch64 MacOS. This PR corrects that. So I've adjusted the bless targets for mir-opt tests, as well as tweaked some of the logic in bootstrap about linker configuration so that we don't try to access the cache of cc/linker configuration when doing the mir-opt builds. While working on that I realized that if I swapped from the `cargo rustc -p std` strategy to `cargo check` on the sysroot, I could use the existing code for check builds to bypass some linker logic. Sweet. But just doing that doesn't work, because then mir-opt tests complain that they can't find an rlib for any of the standard library crates. That happens because nearly all the mir-opt tests are attempting to build `CrateType::Executable`. We already have all the MIR required for mir-opt tests from the rmeta files, but since rustc think we're trying to build an executable it demands we have access to all the upstream monomorphizations that only exist in rlibs, not the meta files in a MIR-only sysroot. So to fix that, I've swapped all the mir-opt tests be passed `--crate-type=rlib`. That works, but leaves us with a few broken mir-opt tests which I've blessed or fixed up; we also lose MIR for some functions so I added `-Clink-dead-code` to paper over that. The inlining changes are because changing the crate-type perturbs the hashes that are compared here to sometimes let us do inlining even in a possibly-recursive call: https://github.com/rust-lang/rust/blob/4cb17b4e78e0540e49d2da884cc621a6bf6f47fa/compiler/rustc_mir_transform/src/inline.rs#L332-L341

rust-log-analyzer · 2024-02-07T06:09:30Z

The job armhf-gnu failed! Check out the build log: (web) (plain)

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

bors · 2024-02-07T06:09:54Z

💔 Test failed - checks-actions

saethlin · 2024-02-07T12:35:12Z

2024-02-07T06:09:14.6844893Z thread 'main' panicked at src/tools/remote-test-client/src/main.rs:310:9:
2024-02-07T06:09:14.6845640Z client.read_exact(&mut header) failed with Connection reset by peer (os error 104)

#97669

@bors retry

bors · 2024-02-07T14:01:24Z

⌛ Testing commit 3c7a8b9 with merge d6c46a2...

bors · 2024-02-07T16:05:11Z

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing d6c46a2 to master...

rust-timer · 2024-02-07T17:22:02Z

Finished benchmarking commit (d6c46a2): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

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

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

Binary size

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

Bootstrap: 664.096s -> 662.763s (-0.20%)
Artifact size: 308.20 MiB -> 308.24 MiB (0.01%)

onur-ozkan · 2024-02-08T16:07:12Z

src/bootstrap/src/core/builder.rs

+        if cmd != "check" {
+            self.configure_linker(
+                compiler,
+                target,
+                &mut cargo,
+                &mut rustflags,
+                &mut rustdocflags,
+                &mut hostflags,
+            );


May I know the motivation behind this if check? It is currently causing redundant recompilations between x anything-but-not-check and x check.

Instead of cmd != "check" which is overly broad, for now it would suffice for this to be "is not a mir-opt test".

The linker configuration prevents cross-compilation. At a minimum, it panics if cc detection for the target didn't succeed.

Instead of cmd != "check" which is overly broad, for now it would suffice for this to be "is not a mir-opt test".

This will still cause recompilations (this time with x test --bless). What are the side effects of running this unconditionally?

If this is run unconditionally, bootstrap will panic when blessing mir-opt tests because the linker configuration innards assumes that we've done cc detection for the target. The panics look like this:

thread 'main' panicked at src/lib.rs:1195:25: no entry found for key note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Build completed unsuccessfully in 0:04:05

rustbot assigned clubby789 Jan 17, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jan 17, 2024

saethlin force-pushed the mir-opt-bless-targets branch 4 times, most recently from ade79c2 to 9099c94 Compare January 21, 2024 01:18

rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Jan 21, 2024

saethlin changed the title ~~Use a better set of targets for blessing mir-opt tests~~ Use the same mir-opt bless targets on all platforms Jan 21, 2024

saethlin marked this pull request as ready for review January 21, 2024 18:51

wesleywiser approved these changes Jan 30, 2024

View reviewed changes

src/bootstrap/src/core/build_steps/test.rs Outdated Show resolved Hide resolved

src/tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved

rustbot assigned wesleywiser and unassigned clubby789 Jan 30, 2024

wesleywiser approved these changes Jan 31, 2024

View reviewed changes

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 Jan 31, 2024

matthiaskrgr mentioned this pull request Feb 3, 2024

Rollup of 7 pull requests #120622

Closed

matthiaskrgr mentioned this pull request Feb 3, 2024

Rollup of 6 pull requests #120623

Closed

matthiaskrgr mentioned this pull request Feb 5, 2024

Rollup of 7 pull requests #120663

Closed

Fixup async_closure_shims mir-opt test

3c7a8b9

saethlin force-pushed the mir-opt-bless-targets branch from 3611d5a to 3c7a8b9 Compare February 7, 2024 04:42

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 Feb 7, 2024

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 Feb 7, 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 Feb 7, 2024

bors mentioned this pull request Feb 7, 2024

Make NonZero constructors generic. #120521

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 7, 2024

bors merged commit d6c46a2 into rust-lang:master Feb 7, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Feb 7, 2024

This was referenced Feb 7, 2024

perform sanity checks before initiating cross-compilation in MirOpt #118050

Closed

Remove the DefPathHash check #117001

Closed

saethlin deleted the mir-opt-bless-targets branch February 7, 2024 16:28

Zalathar mentioned this pull request Feb 8, 2024

"x.py check" dirties the build cache for "x.py build" #120768

Closed

onur-ozkan mentioned this pull request Feb 8, 2024

x test --bless fails to run compiler-builtilns build script: "Failed to find tool. Is `` installed?" #115130

Closed

onur-ozkan reviewed Feb 8, 2024

View reviewed changes

saethlin mentioned this pull request Mar 23, 2024

Enable more mir-opt tests in debug builds #122921

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the same mir-opt bless targets on all platforms #120060

Use the same mir-opt bless targets on all platforms #120060

saethlin commented Jan 17, 2024 •

edited

Loading

rustbot commented Jan 17, 2024

saethlin commented Jan 21, 2024

wesleywiser commented Jan 30, 2024

wesleywiser commented Jan 31, 2024

bors commented Jan 31, 2024

Zalathar commented Feb 5, 2024

saethlin commented Feb 7, 2024 •

edited

Loading

saethlin commented Feb 7, 2024

bors commented Feb 7, 2024

bors commented Feb 7, 2024

rust-log-analyzer commented Feb 7, 2024

bors commented Feb 7, 2024

saethlin commented Feb 7, 2024

bors commented Feb 7, 2024

bors commented Feb 7, 2024

rust-timer commented Feb 7, 2024

onur-ozkan Feb 8, 2024

saethlin Feb 8, 2024

onur-ozkan Feb 8, 2024

saethlin Feb 8, 2024

	if callee_def_id.is_local() {
	// Avoid a cycle here by only using `instance_mir` only if we have
	// a lower `DefPathHash` than the callee. This ensures that the callee will
	// not inline us. This trick even works with incremental compilation,
	// since `DefPathHash` is stable.
	if self.tcx.def_path_hash(caller_def_id).local_hash()
	< self.tcx.def_path_hash(callee_def_id).local_hash()
	{
	return Ok(());
	}

Use the same mir-opt bless targets on all platforms #120060

Use the same mir-opt bless targets on all platforms #120060

Conversation

saethlin commented Jan 17, 2024 • edited Loading

rustbot commented Jan 17, 2024

saethlin commented Jan 21, 2024

wesleywiser commented Jan 30, 2024

wesleywiser commented Jan 31, 2024

bors commented Jan 31, 2024

Zalathar commented Feb 5, 2024

saethlin commented Feb 7, 2024 • edited Loading

saethlin commented Feb 7, 2024

bors commented Feb 7, 2024

bors commented Feb 7, 2024

rust-log-analyzer commented Feb 7, 2024

bors commented Feb 7, 2024

saethlin commented Feb 7, 2024

bors commented Feb 7, 2024

bors commented Feb 7, 2024

rust-timer commented Feb 7, 2024

Overall result: ✅ improvements - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin commented Jan 17, 2024 •

edited

Loading

saethlin commented Feb 7, 2024 •

edited

Loading