GVN: Avoid analyzing references that come from unions or MaybeDangling

[saethlin]

View all comments

This is a possible fix for #163825.

There is a small perf impact, but it seems more like churn than a regression.

Unfortunately I think this change breaks EarlyOtherwiseBranch, because it relies on all of the discriminant reads being from the same Place, and without GVN looking through the no_retag copies produced by EraseDerefTemps they are all from different Places that alias.

[saethlin]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: fade20e (fade20e734a2a69906122ef3506d782e27cbaedd)
Base parent: 33ffc9b (33ffc9b4d83afef2445b585d379d7158cbbd2315)

[rust-timer]

Finished benchmarking commit (fade20e): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@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.2%	[0.1%, 0.3%]	23
Regressions ❌ (secondary)	0.5%	[0.1%, 1.2%]	33
Improvements ✅ (primary)	-0.4%	[-0.6%, -0.1%]	8
Improvements ✅ (secondary)	-0.4%	[-0.7%, -0.2%]	38
All ❌✅ (primary)	0.0%	[-0.6%, 0.3%]	31

Max RSS (memory usage)

Results (primary -0.8%, secondary -4.8%)

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

	mean	range	count
Regressions ❌ (primary)	4.4%	[2.6%, 6.3%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.9%	[-7.0%, -0.5%]	5
Improvements ✅ (secondary)	-4.8%	[-6.2%, -3.5%]	2
All ❌✅ (primary)	-0.8%	[-7.0%, 6.3%]	7

Cycles

Results (primary -2.0%, secondary 3.4%)

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.4%	[3.1%, 3.6%]	2
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.0%	[-2.0%, -2.0%]	1

Binary size

Results (primary -0.1%, secondary -2.1%)

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.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-0.4%	[-1.0%, -0.0%]	14
Improvements ✅ (secondary)	-2.6%	[-11.5%, -0.1%]	5
All ❌✅ (primary)	-0.1%	[-1.0%, 1.0%]	25

Bootstrap: 490.731s -> 491.399s (0.14%)
Artifact size: 408.59 MiB -> 408.68 MiB (0.02%)

[saethlin]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 007547e (007547e70502802536bd001ed0889bca778e81d8)
Base parent: 65efa19 (65efa194201c67ba29b7384255a534d61c2aa3cb)

[rust-timer]

Finished benchmarking commit (007547e): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@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.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	0.6%	[0.2%, 1.0%]	7
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.2%]	3
Improvements ✅ (secondary)	-0.2%	[-0.3%, -0.2%]	3
All ❌✅ (primary)	-0.0%	[-0.3%, 0.7%]	4

Max RSS (memory usage)

Results (primary 0.4%, secondary 2.1%)

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

	mean	range	count
Regressions ❌ (primary)	3.5%	[0.8%, 6.6%]	3
Regressions ❌ (secondary)	2.1%	[2.0%, 2.2%]	2
Improvements ✅ (primary)	-2.8%	[-3.2%, -2.3%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[-3.2%, 6.6%]	6

Cycles

Results (primary 2.4%, secondary 14.0%)

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

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.0%, 2.8%]	2
Regressions ❌ (secondary)	22.3%	[20.2%, 24.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	2.4%	[2.0%, 2.8%]	2

Binary size

Results (primary -0.2%, secondary -0.3%)

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

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.3%, 0.8%]	4
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	3
Improvements ✅ (primary)	-0.3%	[-0.8%, -0.0%]	14
Improvements ✅ (secondary)	-0.8%	[-1.7%, -0.0%]	2
All ❌✅ (primary)	-0.2%	[-0.8%, 0.8%]	18

Bootstrap: 485.904s -> 487.426s (0.31%)
Artifact size: 406.44 MiB -> 406.51 MiB (0.02%)

[saethlin]

That seems in the space of reasonable to me

[saethlin]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8ebb059 (8ebb0595470230a785181ae7dd29e7d4512fcc21)
Base parent: 32dba69 (32dba69d69c5b10ea89a4042de8d0619f7756203)

[rust-timer]

Finished benchmarking commit (8ebb059): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

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

Cycles

Results (primary 0.0%, secondary -2.3%)

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

	mean	range	count
Regressions ❌ (primary)	3.0%	[2.9%, 3.1%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.0%	[-3.0%, -2.9%]	2
Improvements ✅ (secondary)	-2.3%	[-3.0%, -1.6%]	3
All ❌✅ (primary)	0.0%	[-3.0%, 3.1%]	4

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 490.305s -> 488.99s (-0.27%)
Artifact size: 406.56 MiB -> 406.50 MiB (-0.01%)

[saethlin]

This is about unions, not retagging, I think. This is reminiscent of #157346, where fields can introduce references. For most reference cases, I think multiple occurrences of a.b can be unified, except for unions.

I don't think this is about unions. This is another reproducer:

#[inline(never)]
fn demo(p: *mut u8) -> (u8, u8) {
    unsafe {
        let r1: &u8 = std::mem::transmute(p);
        let a = *r1;
        *p = 2;
        let r2: &u8 = std::mem::transmute(p);
        let b = *r2;
        (a, b)
    }
}

fn main() {
    let mut x = 1u8;
    assert_eq!(demo(&raw mut x).1, 2);
}

All of these scenarios: unions, MaybeDangling, and pointer-to-reference transmutes are ways to produce the same provenance structure that defies the reasoning that GVN is trying to use. I suppose we could also have GVN exclude references created by transmuting a pointer? But this really seems like papering over a deeper problem, even according to the comment inside GVN:

rust/compiler/rustc_mir_transform/src/gvn.rs

Lines 858 to 859 in a5c142a

	// An immutable borrow `_x` always points to the same value for the
	// lifetime of the borrow, so we can merge all instances of `*_x`.

Which is true, the problem is that GVN is trying to unify r1 with r2 and their lifetimes do not overlap.

[dianqk]

All of these scenarios: unions, MaybeDangling, and pointer-to-reference transmutes are ways to produce the same provenance structure that defies the reasoning that GVN is trying to use. I suppose we could also have GVN exclude references created by transmuting a pointer?
Which is true, the problem is that GVN is trying to unify r1 with r2 and their lifetimes do not overlap.

If we treat the pointer-to-reference transmute as an opaque value, we should still be able to unify the following dereferences:

let r1: &u8 = std::mem::transmute(p); // an opaque value.
let a = *r1;
let b = *r1; // let b = a;

[dianqk]

Should we have both no-retag and retag tests? Both of them are different MIRs.

View changes since this review

[dianqk]

Is manuallydrop testing with the compile flags? I cannot see _2.0.

Suggested change

	//@ compile-flags: -O -Zinline-mir
	//@ compile-flags: -Zmir-enable-passes=+ReferencePropagation -Zinline-mir -Zub-checks=false

I believe -O is not working.

[dianqk]

Suggested change

	if projection_ty.ty.is_ref()
	if self.ty_may_have_ref(projection_ty.ty)

;p

use std::mem::ManuallyDrop;

union U {
    r: (&'static u8,),
    p: *mut u8,
}

#[inline(never)]
pub fn union_field_ref_tuple(p: *mut u8) -> (u8, u8) {
    let u = U { p };
    unsafe {
        let a = u.r;
        let a = a.0;
        let a = *a;
        *p = 2;
        let b = u.r;
        let b = b.0;
        let b = *b;
        (a, b)
    }
}

#[inline(never)]
pub fn manuallydrop_ref_tuple(p: *mut u8) -> (u8, u8) {
    let m: ManuallyDrop<(&u8,)> = unsafe { std::mem::transmute(p) };
    unsafe {
        let a = *m;
        let a = a.0;
        let a = *a;
        *p = 2;
        let b = *m;
        let b = b.0;
        let b = *b;
        (a, b)
    }
}

[dianqk]

I have a new approach that might be more precise, but it could take a long time to land, so it shouldn't block this PR.

In fact, we can unify (*x).1:

pub fn indirect_deref_1(x: &(&i32,i32), out: fn(i32, i32), clobber: fn()) {
    out(*(*x).0, (*x).1);
    clobber();
    out(*(*x).0, (*x).1);
}

[saethlin]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 37680c3 (37680c364850270f210f2089c7506ecb9953cc5f)
Base parent: 02fb0a1 (02fb0a16311c3411fa373627706da085cd8fbd3c)

[rust-timer]

Finished benchmarking commit (37680c3): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@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)	2.0%	[0.3%, 8.5%]	13
Regressions ❌ (secondary)	1.8%	[0.3%, 6.1%]	10
Improvements ✅ (primary)	-0.3%	[-1.0%, -0.2%]	9
Improvements ✅ (secondary)	-0.6%	[-1.1%, -0.2%]	18
All ❌✅ (primary)	1.1%	[-1.0%, 8.5%]	22

Max RSS (memory usage)

Results (primary 3.0%, secondary -4.5%)

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

	mean	range	count
Regressions ❌ (primary)	3.0%	[1.8%, 4.2%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.5%	[-5.3%, -3.7%]	2
All ❌✅ (primary)	3.0%	[1.8%, 4.2%]	4

Cycles

Results (primary 4.5%, secondary 2.3%)

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

	mean	range	count
Regressions ❌ (primary)	4.5%	[4.0%, 5.1%]	2
Regressions ❌ (secondary)	3.7%	[2.1%, 5.8%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-3.4%, -3.4%]	1
All ❌✅ (primary)	4.5%	[4.0%, 5.1%]	2

Binary size

Results (primary 0.7%, secondary 1.2%)

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

	mean	range	count
Regressions ❌ (primary)	5.5%	[5.5%, 5.5%]	1
Regressions ❌ (secondary)	3.0%	[0.5%, 5.5%]	2
Improvements ✅ (primary)	-0.5%	[-1.1%, -0.1%]	4
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	3
All ❌✅ (primary)	0.7%	[-1.1%, 5.5%]	5

Bootstrap: 489.75s -> 490.638s (0.18%)
Artifact size: 409.00 MiB -> 409.16 MiB (0.04%)