Enable test-compare-mode on PR builder

[Mark-Simulacrum]

This moves it over from the nopt builder (taking ~3 hours) to the PR builder (taking ~2 hours).

Primarily this is done so that we can get early warning for changes to the compare-mode output before we hit bors.

[rust-highfive]

r? @alexcrichton

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

[Mark-Simulacrum]

cc @rust-lang/infra

[Centril]

(We recently made the mir-opt tests take way less time so we should have the budget for this, which would reduce a lot of pain and failing rollups.)

[pietroalbini]

Not sure how much this is worth, especially because the PR builder is run multiple times in parallel (at the moment of writing this there are 3 running, during peak hours it's not unusual to see 9 running concurrently). That builder slowing down it is going to impact our CI capacity much more than others.

[alexcrichton]

As someone who's not in the know, could you explain what this does? I'm not sure what "test-compare-mode" is and how much time it's shifting over.

[Centril]

@alexcrichton We have --compare-mode=nll which runs the UI tests twice, once with migrate mode (always the case) and with NLL (this is added). This generates some .nll.stderr files.

Unfortunately, we only run the UI tests with NLL in the auto builder rather than the PR builder which means that rollups sometimes fail because of it (due to the .nll.stderr files) and is rather annoying (recently, it happened to me that 2 rollups failed in a day).

Testing --compare-mode=nll took 348.566 seconds extra when I tested this right now (i7-7700K, 64GB RAM). However, we recently landed #64344 which made the mir-opt tests take way less (197s -> 2.85s) on @eddyb's machine which is substantially faster than mine. Given the speedup to mir-opt, I think we have the budget for this.

I should note that migrate mode is going away in 15-ish days and then we will use NLL everywhere then. However, --compare-mode=polonius is being added soon I hear so we'll he in a similar situation then as well.

[Mark-Simulacrum]

I should also mention that particularly with this being removed entirely in the next few weeks (and, I presume, for some time before we gate CI on it) I don't feel strongly at all about this.

[alexcrichton]

Thanks for the explanation of what this is doing, but do we have any idea how long this takes on CI? That's sort of the only timing measurement we're worried about here. (the compiler builds in 10 minutes on my machine, but that's not too relevant for CI where it takes 1.5hrs).

If this is already on its way out, should we just delete the test suite flag outright?

[Mark-Simulacrum]

Looks like it takes ~15 minutes total based on https://dev.azure.com/rust-lang/e71b0ddf-dd27-435a-873c-e30f86eea377/_apis/build/builds/8233/logs/1112 but I'd need to investigate a bit more to be confident (looked for path: Some("src/test/ui") which there appeared to only be one of, so we're running them in the same "block" it looks like).

[Centril]

If this is already on its way out, should we just delete the test suite flag outright?

(Only once we have removed migrate mode -- assuming we're not going to re-purpose it for polonius; wouldn't want accidents to happen in the few days it has left to live.)

[Mark-Simulacrum]

Going to close this out actually since we're almost to the point of removing AST/HIR borrowck. We can revisit if/when polonius based compare mode becomes more prevalent, though I personally hope we can try and avoid that for a long while and make the switch more quickly.