GVN: Use the borrows only if they are always live

[dianqk]

Fixes #141313 and #141313 (comment). This PR forbids introducing borrows outside their lifetime.

c8fbb62 fixes #130853.

I agree with what #147844 (comment) mentioned:

We can reason with the value behind a reference because it is UB to directly assign to the underlying local while the reference is live. We allow creating new derefs, this means extending the liveness of references, so we are creating UB.

r? cjgillot

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[dianqk]

@cjgillot I just found this still introduces copy overlapping.

[dianqk]

@bors try @rust-timer queue

[cjgillot]

I'd have kept introducing Value::Projection(Deref) as exists, but forbidden introducing Deref projections in try_as_place. What do you think?

[rust-bors]

☀️ Try build successful (CI)
Build commit: efb5f98 (efb5f98020c7392070aae01c719ed60b78144ff5, parent: c6efb9019b3169fc672248339dbbf13e6a134de3)

[rust-timer]

Finished benchmarking commit (efb5f98): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@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.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.3%]	3
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	-0.1%	[-0.4%, 0.8%]	4

Max RSS (memory usage)

Results (primary 4.1%, secondary 0.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	4.1%	[2.8%, 5.3%]	2
Regressions ❌ (secondary)	0.8%	[0.8%, 0.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.1%	[2.8%, 5.3%]	2

Cycles

Results (secondary 3.2%)

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

Binary size

Results (primary 0.0%, secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.0%, 1.0%]	11
Regressions ❌ (secondary)	0.0%	[0.0%, 0.2%]	15
Improvements ✅ (primary)	-0.1%	[-0.5%, -0.0%]	17
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	9
All ❌✅ (primary)	0.0%	[-0.5%, 1.0%]	28

Bootstrap: 472.659s -> 474.116s (0.31%)
Artifact size: 390.57 MiB -> 391.13 MiB (0.14%)

[dianqk]

I'd have kept introducing Value::Projection(Deref) as exists, but forbidden introducing Deref projections in try_as_place. What do you think?

Oh, I think this is a great insight. IIUC, I can remove invalid_derefs and introduce a borrow-reaching definition that is used in try_as_place. This should allow us to preserve derefs in the RPO A-B(terminator is call)-C.

[dianqk]

I'd have kept introducing Value::Projection(Deref) as exists, but forbidden introducing Deref projections in try_as_place. What do you think?

Oh, I think this is a great insight. IIUC, I can remove invalid_derefs and introduce a borrow-reaching definition that is used in try_as_place. This should allow us to preserve derefs in the RPO A-B(terminator is call)-C.

Hmm, I still need to handle this in simplify_binary_inner​ to check whether *_1​ is still valid at the location of *_2​. There might be other codes I’ve missed. I can also check the deref in get​.

Actually, that's not enough. insert​ reuses VnIndex​ when values are equal. For that, I could add a provenance, but I don't see what difference it makes not to introduce new derefs. Maybe do this when we can know if the immutable borrow is in its lifetime scope at the location.

[dianqk]

I'd have kept introducing Value::Projection(Deref) as exists, but forbidden introducing Deref projections in try_as_place. What do you think?

I changed it to forbid introducing borrows outside their lifetime. And c8fbb62 should fix #130853.

I have another experimental patch, 6e2ecde, using live variables, but this may introduce miscompile, and I won't add it.

@rustbot review

[dianqk]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 2b9aa76 (2b9aa76acf43f7c466566e82eb9c30d4cd75d243, parent: 377656d3dd3f9c23a9c8713e163f4365a5261a84)

[rust-timer]

Finished benchmarking commit (2b9aa76): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@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.3%	[0.1%, 0.6%]	8
Regressions ❌ (secondary)	0.5%	[0.2%, 1.4%]	24
Improvements ✅ (primary)	-1.0%	[-2.5%, -0.2%]	3
Improvements ✅ (secondary)	-0.4%	[-2.4%, -0.1%]	8
All ❌✅ (primary)	-0.0%	[-2.5%, 0.6%]	11

Max RSS (memory usage)

Results (primary 2.2%, secondary 1.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.8%	[2.4%, 5.3%]	2
Regressions ❌ (secondary)	1.1%	[1.1%, 1.1%]	1
Improvements ✅ (primary)	-1.2%	[-1.2%, -1.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.2%	[-1.2%, 5.3%]	3

Cycles

Results (primary -2.2%, secondary 0.1%)

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

Binary size

Results (primary -0.1%, secondary 0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.1%]	18
Regressions ❌ (secondary)	1.6%	[0.0%, 15.0%]	10
Improvements ✅ (primary)	-0.3%	[-1.6%, -0.0%]	19
Improvements ✅ (secondary)	-0.2%	[-1.6%, -0.0%]	11
All ❌✅ (primary)	-0.1%	[-1.6%, 0.1%]	37

Bootstrap: 471.42s -> 472.371s (0.20%)
Artifact size: 389.04 MiB -> 389.08 MiB (0.01%)

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[dianqk]

@rustbot author

[dianqk]

I realized that we are propagating the borrows of non-SSA locals. This is why we use borrows outside their lifetime.
I think #150485 is a better resolution.

[rust-bors]

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

[dianqk]

Superseded by #150485.