Skip to content
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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Oct 8, 2023

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.

@rustbot
Copy link
Collaborator

rustbot commented Oct 8, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 8, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 8, 2023

Changes to the size of AST and/or HIR nodes.

cc @nnethercote

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Oct 8, 2023

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?

| ty::RawPtr(..)
| ty::Char
| ty::Ref(..)
| ty::Closure(..) => true,
Copy link
Member

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.

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)
Copy link
Member

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) => {
Copy link
Member

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;
Copy link
Member

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?

@the8472
Copy link
Member Author

the8472 commented Oct 8, 2023

I think the test failure indicates that I've already broken something because rustc_graphviz only depends on std and doesn't have any unsafe, so its behavior should be unchanged.

@@ -1,7 +1,7 @@
// check-pass
// compile-flags: -Zhir-stats
// only-x86_64

// ignore-stage1
Copy link
Contributor

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?

Copy link

@riking riking Mar 27, 2024

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

@the8472
Copy link
Member Author

the8472 commented Oct 8, 2023

My approach probably is too naive. Apparently layout_of gets called for generic &[T] (with unresolved T)? Having different layouts for &[T] and &[<concrete type>] where one allows niches and the other doesn't seems like it would cause issues.

Though I'm a bit surprised that all the stage 2 UI/codegen/std tests pass and it "merely" fails in rustc_graphviz.

@the8472 the8472 marked this pull request as draft October 8, 2023 22:50
@RalfJung
Copy link
Member

RalfJung commented Oct 9, 2023

Yes layout_of gets called on generic types, and if it returns a result then it must be the case that instantiating the generics must not change the resulting layout. This way we can know some layout facts even before monomorphization.

With your PR this means that &[T] must return TooGeneric if it doesn't know whether T is a ZST or not.

@cjgillot
Copy link
Contributor

Having &[T] look at the layout of T will definitely create cycles during layout computation. For instance: struct A<'a> { x: &'a [A<'a>] }. That is probably impossible.

Could we cheat by adding the annotation in codegen, on the code we generate for Len MIR statement? Just the proper range attribute. At that point we have all the layout info we need.

@the8472
Copy link
Member Author

the8472 commented Oct 11, 2023

For instance: struct A<'a> { x: &'a [A<'a>] }. That is probably impossible.

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 x is a reference A is known to be non-zero. That's similar to the naive layout computation in #113166, but simpler.

Just the proper range attribute.

Yeah, that's the fallback solution if I can't get this to work. It'll be simpler but lose the niches

@cjgillot cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2023
@bors
Copy link
Contributor

bors commented Dec 26, 2023

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

@the8472 the8472 force-pushed the slice-ref-len-validity branch from f18bdd5 to 9fb9beb Compare March 24, 2024 00:43
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@the8472 the8472 force-pushed the slice-ref-len-validity branch from dc27f9d to e47adf2 Compare March 24, 2024 22:30
@rust-log-analyzer

This comment has been minimized.

@the8472
Copy link
Member Author

the8472 commented Mar 24, 2024

Back to a broken rustc_graphviz . I'm not sure how I'm breaking it. Maybe something about enum discriminants or about &str....

@Dylan-DPC
Copy link
Member

@the8472 any updates on this? thanks

@the8472 the8472 force-pushed the slice-ref-len-validity branch from c0e2e89 to f6c6fd2 Compare October 6, 2024 21:32
@rust-log-analyzer

This comment has been minimized.

@the8472 the8472 force-pushed the slice-ref-len-validity branch from f6c6fd2 to 907a078 Compare October 8, 2024 23:10
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 10, 2024

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

@the8472 the8472 force-pushed the slice-ref-len-validity branch from 2b66bb6 to d1794d3 Compare October 10, 2024 19:23
@rust-log-analyzer

This comment has been minimized.

@the8472 the8472 force-pushed the slice-ref-len-validity branch from d1794d3 to 6671c90 Compare October 10, 2024 20:27
@the8472
Copy link
Member Author

the8472 commented Oct 10, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 10, 2024
@bors
Copy link
Contributor

bors commented Oct 10, 2024

⌛ Trying commit 6671c90 with merge 9a4ff20...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 10, 2024
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.
@bors
Copy link
Contributor

bors commented Oct 10, 2024

☀️ Try build successful - checks-actions
Build commit: 9a4ff20 (9a4ff2049a8afe03a213bd14c5a99d81198b3df3)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9a4ff20): comparison URL.

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

Benchmarking 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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
0.4% [0.1%, 1.6%] 149
Regressions ❌
(secondary)
0.8% [0.2%, 3.1%] 60
Improvements ✅
(primary)
-1.7% [-1.7%, -1.7%] 1
Improvements ✅
(secondary)
-1.3% [-6.6%, -0.3%] 9
All ❌✅ (primary) 0.4% [-1.7%, 1.6%] 150

Max RSS (memory usage)

Results (primary -1.1%, secondary -0.9%)

This 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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.8% [3.2%, 4.5%] 3
Improvements ✅
(primary)
-1.1% [-1.6%, -0.8%] 4
Improvements ✅
(secondary)
-1.8% [-4.5%, -0.8%] 17
All ❌✅ (primary) -1.1% [-1.6%, -0.8%] 4

Cycles

Results (primary 0.6%, secondary 0.4%)

This 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.

mean range count
Regressions ❌
(primary)
1.0% [0.8%, 1.1%] 6
Regressions ❌
(secondary)
1.5% [1.0%, 2.1%] 3
Improvements ✅
(primary)
-1.6% [-1.6%, -1.6%] 1
Improvements ✅
(secondary)
-3.1% [-3.1%, -3.1%] 1
All ❌✅ (primary) 0.6% [-1.6%, 1.1%] 7

Binary size

Results (primary 0.3%, secondary 1.1%)

This 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.

mean range count
Regressions ❌
(primary)
0.3% [0.0%, 1.3%] 31
Regressions ❌
(secondary)
1.1% [0.0%, 1.7%] 43
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.3% [0.0%, 1.3%] 31

Bootstrap: 781.528s -> 782.278s (0.10%)
Artifact size: 331.97 MiB -> 332.38 MiB (0.12%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 11, 2024
@the8472 the8472 force-pushed the slice-ref-len-validity branch from 6671c90 to 598d653 Compare October 12, 2024 17:39
@@ -88,8 +89,22 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

fn size_to_bits(size: Size) -> u128 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.Size.html#method.bits and bits_usize already exist, maybe put a method over with them instead? Or if you're already relying on the 2⁴⁸ limit anyway, maybe just use size.bits() and deal in u64?

// Try to display a sensible error with as much information as possible.
let skeleton_string = |ty: Ty<'tcx>, sk: Result<_, &_>| match sk {
Ok(SizeSkeleton::Pointer { tail, known_size: Some(size), .. }) => {
format!("{} bits, pointer to `{tail}`", size_to_bits(size))
}
Ok(SizeSkeleton::Pointer { tail, .. }) => format!("pointer to `{tail}`"),
Ok(SizeSkeleton::Known(size, _)) => {
if let Some(v) = u128::from(size.bytes()).checked_mul(8) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...oh, looks like down here also should be using such a thing

@@ -20,7 +21,7 @@ pub fn chunks4(x: &[u8]) -> &[[u8; 4]] {
// CHECK-LABEL: @chunks4_with_remainder
#[no_mangle]
pub fn chunks4_with_remainder(x: &[u8]) -> (&[[u8; 4]], &[u8]) {
// CHECK-DAG: and i64 %x.1, -4
// CHECK-DAG: and i64 %x.1, 9223372036854775804
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ymmv: You can use a numeric pattern to writes this in hex, which I think is easier to read here:

Suggested change
// CHECK-DAG: and i64 %x.1, 9223372036854775804
// CHECK-DAG: and i64 %x.1, [[#0x7FFFFFFFFFFFFFFC]]

(I learned this trying to make a similar change in https://github.com/rust-lang/rust/pull/122926/files#diff-1f5fbc02acba64b04fe4b9ccdb433dcaa57f5ff617088895a87c03ff104ef03cR24 🙂)

@bors
Copy link
Contributor

bors commented Oct 20, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.