Revert "Rollup merge of #154344 - dianqk:update-llvm, r=nikic"

[Kobzol]

View all comments

This reverts commit d0fcb41, reversing
changes made to 85ddb8e.

Debugging perf. hit from #154384.

Seems like this PR caused some large perf. regressiosn in #154384 :(

CC @dianqk

[Kobzol]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 6c20012 (6c2001265a930ef1878c1a5a9575a34044358980, parent: fda6d37bb88ee12fd50fa54d15859f1f91b74f55)

[rust-timer]

Finished benchmarking commit (6c20012): comparison URL.

Overall result: ✅ improvements - no action needed

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.

@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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.3%	[-3.2%, -0.2%]	28
Improvements ✅ (secondary)	-1.2%	[-5.0%, -0.2%]	13
All ❌✅ (primary)	-1.3%	[-3.2%, -0.2%]	28

Max RSS (memory usage)

Results (secondary 7.5%)

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

Cycles

Results (primary -2.3%, secondary -3.7%)

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
Improvements ✅ (primary)	-2.3%	[-2.7%, -2.1%]	6
Improvements ✅ (secondary)	-3.7%	[-4.0%, -3.4%]	2
All ❌✅ (primary)	-2.3%	[-2.7%, -2.1%]	6

Binary size

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

Bootstrap: 485.109s -> 484.795s (-0.06%)
Artifact size: 395.05 MiB -> 397.03 MiB (0.50%)

[rustbot]

r? @cuviper

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @cuviper

[rustbot]

⚠️Warning⚠️

• Some commits in this PR modify submodules.

  If this was not intentional, see I changed a submodule on accident in the rustc dev guide.

• There are issue links (such as #123) in the commit messages of the following commits.
  Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.

  • e724c4f

[nikic]

My first guess would be llvm/llvm-project@4dfb4b6. If that's the case, backporting llvm/llvm-project#186723 should fix it.

[dianqk]

My first guess would be llvm/llvm-project@4dfb4b6. If that's the case, backporting llvm/llvm-project#186723 should fix it.

PR: #154484

[nikic]

Hm, it looks like that fixes part of the regression, but not all of it...

[dianqk]

Hm, it looks like that fixes part of the regression, but not all of it...

The other part is being reverted: #154468.

[Kobzol]

Yeah, I think that between this PR and #154200, the perf. hit should be mostly gone.

Should we r+ this, or do you want to wait for some LLVM update?

[nikic]

What I meant is that the perf run on this PR and on #154484 don't have the same result. In particular this one has -5% on large-workspace, but the other one doesn't. So it seems like there may be another regression in the LLVM update? Or am I misinterpreting something here?

[Kobzol]

Yeah, I assume that there are other LLVM regressions that the one that was attempted in #154484. I'd thus suggest to land this revert, as the regressions are quite big on a bunch of real-world crates, and then re-land the LLVM changes incrementally, while doing perf. runs on them to ensure that the regression is not presnet.

[nikic]

I put up #154511 to fully revert the FastISel change. It is solely responsible for the regression.

Looks like the mitigation from llvm/llvm-project#186723 only partially mitigated the impact of that change.

[rust-bors]

☔ The latest upstream changes (presumably #154511) made this pull request unmergeable. Please resolve the merge conflicts.

[Kobzol]

Superseded by #154511.