GVN: transmute adts to their fields if a field projection is immediately transmuted anyway#153085
GVN: transmute adts to their fields if a field projection is immediately transmuted anyway#153085oli-obk wants to merge 4 commits intorust-lang:mainfrom
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
@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: transmute adts to their fields if a field projection is immediately transmuted anyway
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (de634ba): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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 @bors rollup=never 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 5.3%, secondary 6.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (primary -0.1%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 478.569s -> 481.658s (0.65%) |
| return Some(value); | ||
| } | ||
| } else if let Some((f_idx, field_ty)) = | ||
| self.value_is_all_in_one_field(self.ty(field_value), FIRST_VARIANT) |
There was a problem hiding this comment.
Should this be another VariantIdx if field_value is a downcast?
| self.get(field_value) | ||
| && let downcast_ty = self.ty(downcast_value) | ||
| && let Ok(downcast_layout) = self.ecx.layout_of(downcast_ty) | ||
| && let Ok(projected_layout) = self.ecx.layout_of(self.ty(value)) |
There was a problem hiding this comment.
| && let Ok(projected_layout) = self.ecx.layout_of(self.ty(value)) | |
| && let Ok(projected_layout) = self.ecx.layout_of(from) |
|
Very cool! I like that we can do this in mir so it can handle things like I really don't feel confident that I know the nuances of GVN enough to comfortable reviewing this, though, so |
| StorageDead(_7); | ||
| StorageLive(_9); | ||
| _9 = copy _8 as *mut u8 (Transmute); | ||
| _10 = alloc::alloc::__rust_dealloc(move _9, move _5, move _6) -> [return: bb3, unwind unreachable]; |
There was a problem hiding this comment.
Hmm, it means we didn't know the NonNull-ness before, and we don't (currently) elide transmutes like this when the middle type is more specific than the source or destination in order to preserve that extra niche information for the backend.
See discussion in #152702 (comment) ; I don't think doing something about it needs to be this PR.
follow-up to #152702
commits best reviewed individually.
Basically does
in various forms, including going from an
Optiondirectly to a value in itsSomevariant if the value got niche optimized into theOptionr? @scottmcm
cc @cjgillot