-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add range metadata to slice lengths #116542
base: master
Are you sure you want to change the base?
Conversation
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
Changes to the size of AST and/or HIR nodes. cc @nnethercote |
This comment has been minimized.
This comment has been minimized.
Note that doing this for raw slice pointers would be unsound, since slice_from_raw_parts is safe. So this PR must be introducing (for the first time) a difference in metadata validity for raw pointers vs references. Are we sure we want that? |
compiler/rustc_middle/src/ty/sty.rs
Outdated
| ty::RawPtr(..) | ||
| ty::Char | ||
| ty::Ref(..) | ||
| ty::Closure(..) => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closure
could be a ZST I think, if the environment is empty.
compiler/rustc_middle/src/ty/sty.rs
Outdated
ty::Tuple(tys) => tys.iter().any(|ty| ty.is_trivially_non_zst(tcx)), | ||
ty::Adt(def, args) => def.all_fields().any(|field| { | ||
let ty = field.ty(tcx, args); | ||
ty.is_trivially_non_zst(tcx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is non-obvious and deserves a comment. It relies on Result<(), (!, i32)>
not removing the Err
variant entirely (which plausibly it could do due to it being uninhabited -- but that would also cause issues in MIR).
return Err(error(cx, LayoutError::Unknown(pointee))); | ||
}; | ||
|
||
if !ty.is_unsafe_ptr() { | ||
match pointee.kind() { | ||
ty::Slice(element) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So &[i32]
is getting the optimization but &(bool, [i32])
is not? That seems odd?
ty::Slice(element) => { | ||
let mut metadata = scalar_unit(Int(dl.ptr_sized_integer(), false)); | ||
if !ty.is_unsafe_ptr() && !element.is_trivially_non_zst(tcx) { | ||
metadata.valid_range_mut().end = dl.ptr_sized_integer().signed_max() as u128; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have this logic duplicated here?
I think the test failure indicates that I've already broken something because |
tests/ui/stats/hir-stats.rs
Outdated
@@ -1,7 +1,7 @@ | |||
// check-pass | |||
// compile-flags: -Zhir-stats | |||
// only-x86_64 | |||
|
|||
// ignore-stage1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is needed temporarily, can you add a comment saying that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
write this as a cfg(bootstrap) so that it gets picked up during the version bump
My approach probably is too naive. Apparently Though I'm a bit surprised that all the stage 2 UI/codegen/std tests pass and it "merely" fails in rustc_graphviz. |
Yes With your PR this means that |
Having Could we cheat by adding the annotation in codegen, on the code we generate for |
We don't necessarily need the full layout if we limit ourselves to adding an isize::MAX range annotation - computing more accurate, size-based ranges would require the layout - then we only need to know whether A is certainly ZST or non-ZST. since
Yeah, that's the fallback solution if I can't get this to work. It'll be simpler but lose the niches |
☔ The latest upstream changes (presumably #119258) made this pull request unmergeable. Please resolve the merge conflicts. |
f18bdd5
to
9fb9beb
Compare
This comment has been minimized.
This comment has been minimized.
9fb9beb
to
dc27f9d
Compare
This comment has been minimized.
This comment has been minimized.
dc27f9d
to
e47adf2
Compare
This comment has been minimized.
This comment has been minimized.
Back to a broken rustc_graphviz . I'm not sure how I'm breaking it. Maybe something about enum discriminants or about |
☔ The latest upstream changes (presumably #122849) made this pull request unmergeable. Please resolve the merge conflicts. |
e47adf2
to
c054bbc
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Add range metadata to slice lengths This adds range information to the slice-len in fat pointers if we can conservatively determine that the pointee is not a ZST without having to normalize the pointee type. I only intended to pass the `!range` to llvm but apparently this also lets the length in fat pointers be used for its niches 😅. Ideally this would use the naive-layout computation from rust-lang#113166 to calculate a better approximation of the pointee size, but that PR got reverted.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (cbf560f): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 666.819s -> 669.852s (0.45%) |
The layout code itself barely even shows up in cachegrind diffs. So I assume that the regressions are due to LLVM doing more work or worse codegen. Perhaps it is better after all to give expanding the already-existing niche in references another try. That'll have to be a target-dependent optimization though. |
c054bbc
to
d5d1e65
Compare
This comment has been minimized.
This comment has been minimized.
…ot be ZST # Conflicts: # compiler/rustc_hir/src/hir.rs # compiler/rustc_ty_utils/src/layout.rs # tests/ui/stats/hir-stats.stderr
d5d1e65
to
c0e2e89
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Add range metadata to slice lengths This adds range information to the slice-len in fat pointers if we can conservatively determine that the pointee is not a ZST without having to normalize the pointee type. I only intended to pass the `!range` to llvm but apparently this also lets the length in fat pointers be used for its niches 😅. Ideally this would use the naive-layout computation from rust-lang#113166 to calculate a better approximation of the pointee size, but that PR got reverted.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (254d910): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 674.128s -> 677.312s (0.47%) |
This adds range information to the slice-len in fat pointers if we can conservatively determine that the pointee is not a ZST without having to normalize the pointee type.
I only intended to pass the
!range
to llvm but apparently this also lets the length in fat pointers be used for its niches 😅.Ideally this would use the naive-layout computation from #113166 to calculate a better approximation of the pointee size, but that PR got reverted.