Repository navigation
Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
GVN: Only analyze dereferences if the reference is a function argument
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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 @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
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.
CyclesResults (primary -2.0%, secondary 3.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 490.731s -> 491.399s (0.14%) |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
GVN: Only analyze dereferences if the reference is a function argument
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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 @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
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.
CyclesResults (primary 2.4%, secondary 14.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.2%, secondary -0.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 485.904s -> 487.426s (0.31%) |
|
That seems in the space of reasonable to me |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
If we treat the pointer-to-reference transmute as an opaque value, we should still be able to unify the following dereferences: |
| @@ -0,0 +1,65 @@ | |||
| //@ test-mir-pass: GVN | |||
| //@ compile-flags: -O -Zinline-mir | |||
There was a problem hiding this comment.
Is manuallydrop testing with the compile flags? I cannot see _2.0.
| //@ compile-flags: -O -Zinline-mir | |
| //@ compile-flags: -Zmir-enable-passes=+ReferencePropagation -Zinline-mir -Zub-checks=false |
I believe -O is not working.
| let projection_ty = place_ty.projection_ty(self.tcx, proj); | ||
| // References that we load from a union field or MaybeDangling cannot be unified. | ||
| // See https://github.com/rust-lang/rust/issues/163825 | ||
| if projection_ty.ty.is_ref() |
There was a problem hiding this comment.
| 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)
}
}There was a problem hiding this comment.
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);
}3906272 to
fe10c74
Compare
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
GVN: Avoid analyzing references that come from unions or MaybeDangling
This comment has been minimized.
This comment has been minimized.
|
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 @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
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.
CyclesResults (primary 4.5%, secondary 2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.7%, secondary 1.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 489.75s -> 490.638s (0.18%) |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
GVN: Avoid analyzing references that come from unions or MaybeDangling
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (c21e106): 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 @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.6%, secondary 1.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeResults (primary 0.7%, secondary 1.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 490.17s -> 488.949s (-0.25%) |
|
☔ The latest upstream changes (presumably #165234) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
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.