Make x test --stage 2 compiler/rustc_XXX faster to run

[jyn514]

Previously, bootstrap unconditionally rebuilt the stage 2 compiler,
even if it had previously built stage 1. This changes it to reuse stage 1 if possible.
In particular, it no longer runs the following step:

Building stage1 compiler artifacts (x86_64-unknown-linux-gnu(x86_64-unknown-linux-gnu) -> x86_64-unknown-linux-gnu(x86_64-unknown-linux-gnu))

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[jyn514]

Oh, I just realized - we should probably audit all the CI jobs to make sure they're building at most 2 compilers. It's very easy to call compiler() instead of compiler_for() by accident, and I don't think there's any reason it's necessary for dist artifacts? Do we even need compiler() at all?

[Mark-Simulacrum]

I'm not sure I have in-cache enough the details to be certain, but I will say that any cross-compiling CI I think today goes up to stage 2 for the build triple and then cross-compiles from there (maybe I am wrong, I recall some work in that area a few years back, but it's been a while).

It seems like a good idea overall though.

I am not sure whether compiler/compiler_for is a meaningful + worthwhile distinction; I think the best way to find out is probably to try to unify and see what goes wrong. I suspect the intent behind the addition was largely the typical "it seems like this one place wants something slightly different" that perhaps was more of a patchwork fix than done with long-term correctness, so it's unlikely to be super principled I expect in terms of where it's used and such.

[Mark-Simulacrum]

@bors r+ rollup=iffy

Going to approve this change, though I wouldn't be too surprised if it bounces off CI in some obscure way :) I think it's not hurtful and may be helpful.

[bors]

📌 Commit 070bd64 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 070bd64 with merge 74582ac...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 74582ac to master...

[rust-timer]

Finished benchmarking commit (74582ac): comparison url.

Summary:

• Primary benchmarks: 😿 relevant regression found
• Secondary benchmarks: 🎉 relevant improvement found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	1	0	0	1	1
mean2	1.5%	N/A	N/A	-0.7%	1.5%
max	1.5%	N/A	N/A	-0.7%	1.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

1. number of relevant changes ↩

2. the arithmetic mean of the percent change ↩

[jyn514]

I am not sure whether compiler/compiler_for is a meaningful + worthwhile distinction; I think the best way to find out is probably to try to unify and see what goes wrong. I suspect the intent behind the addition was largely the typical "it seems like this one place wants something slightly different" that perhaps was more of a patchwork fix than done with long-term correctness, so it's unlikely to be super principled I expect in terms of where it's used and such.

I tried changing this and it went ... not well. Removing compiler altogether gives 71 build errors, and changing it to alias compiler_for(stage, host, host) fails a bunch of the tests, including some that look like real issues:

---- builder::tests::dist::dist_with_targets_and_hosts stdout ----
thread 'builder::tests::dist::dist_with_targets_and_hosts' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
 [
     Rustc {
         compiler: Compiler {
<            stage: 1,
>            stage: 2,
             host: A,
>        },
>    },
>    Rustc {
>        compiler: Compiler {
>            stage: 2,
>            host: B,
         },
     },
 ]

', src/bootstrap/builder/tests.rs:314:9

Looking at the call sites, they usually look like this:

            fn make_run(run: RunConfig<'_>) {
                run.builder.ensure($name {
                    compiler: run.builder.compiler(run.builder.top_stage, run.builder.config.build),
                    target: run.target,
                    extra_features: Vec::new(),
                });
            }

so I think the "proper" fix is to pass run.target into compiler_for in most of these cases; at very least, we can't assume it's always host.

Don't have time for that soon, but I'll open an issue and hopefully someone grabs it.

[pnkfelix]

Visiting for weekly performance triage. It seems like syn-1.0.89 opt full suddenly became much noisier around 2022-04-17, and I do not know why:

@rustbot label: +perf-regression-triaged