Bounds-check with PtrMetadata instead of Len in MIR #133734

scottmcm · 2024-12-02T06:57:25Z

Rather than emitting Len(*_n) in array index bounds checks, emit PtrMetadata(copy _n) instead -- with some asterisks for arrays and &mut that need it to be done slightly differently.

We're getting pretty close to removing Len entirely, actually. I think just one more PR after this (for slice drop shims).

r? mir

rustbot · 2024-12-02T06:57:35Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

scottmcm · 2024-12-02T07:00:26Z

tests/mir-opt/dataflow-const-prop/slice_len.main.DataflowConstProp.32bit.panic-abort.diff

-         _7 = Len((*_2));
-         _8 = Lt(copy _6, copy _7);
-         assert(move _8, "index out of bounds: the length is {} but the index is {}", move _7, copy _6) -> [success: bb1, unwind unreachable];
-+         _7 = const 3_usize;
-+         _8 = const true;
-+         assert(const true, "index out of bounds: the length is {} but the index is {}", const 3_usize, const 1_usize) -> [success: bb1, unwind unreachable];


I ended up just deleting this test, which seemed fine to me because GVN can do similar things to this, and DataflowConstProp is off in release anyway.

scottmcm · 2024-12-02T07:05:14Z

tests/mir-opt/pre-codegen/slice_iter.range_loop.PreCodegen.after.panic-abort.mir

-        _11 = Len((*_1));
-        _12 = Lt(copy _10, copy _11);
-        assert(move _12, "index out of bounds: the length is {} but the index is {}", move _11, copy _10) -> [success: bb6, unwind unreachable];
+        _11 = Lt(copy _10, copy _3);
+        assert(move _11, "index out of bounds: the length is {} but the index is {}", copy _3, copy _10) -> [success: bb6, unwind unreachable];


And overall the MIR seems slightly better after this PR, since [T]::len had already moved to PtrMetadata, and thus using it here too means that GVN can dedup the PtrMetadata from the bounds check with the one from the .len() call in the source.

scottmcm · 2024-12-02T09:01:25Z

Looks like I need to rethink this, because it seems borrowck and initialization checking are somehow coupled to this lowering.

EDIT: actually, maybe I just need a fake read.

rustbot · 2024-12-03T18:54:01Z

The Miri subtree was changed

cc @rust-lang/miri

scottmcm · 2024-12-03T19:27:41Z

src/tools/miri/tests/fail/both_borrows/buggy_as_mut_slice.rs

-    //~[stack]^ ERROR: /write access .* tag does not exist in the borrow stack/
+    //~[stack]^ ERROR: /trying to retag .+ for SharedReadOnly permission .+ tag does not exist in the borrow stack for this location/


@RalfJung I think what's happened here is that SB is now failing "on" the read of the slice length for the bounds check, before it gets to trying to write, thus it mentioning SharedReadOnly instead of "write access", which seems at least plausibly fine. But please double-check that I didn't blow this up in some horrible way 😬

Hm, yeah as you say it seems to fail on a read / retag-for-read now, instead of a write. It is a bit surprising that this would change now. How did the MIR change here?

Would be good to add a slice index expression in mir-opt/retag.rs so that we can see how this looks like with retagging.

Because it's a &mut [_], it changed from doing the bounds check as

_2 = Len(*_1); _3 = Lt(const 1_usize, _2) assert(_3, …);

to

_4 = &raw const *_1; _2 = PtrMetadata(move _4); _3 = Lt(const 1_usize, _2) assert(_3, …);

(It's not just PtrMetadata(copy _1) because there could be a projection [like Len((*_1).3)] and because that copy on a &mut ends up breaking borrowck.)

Ah, yeah the &raw const here is relevant for Stacked Borrows -- it basically acts as a read on the entire slice. That is a bit unfortunate since so far, for slice accesses, we didn't do such a whole-slice retag. I think ideally we would not add a retag to these raw reborrows.

Is there any way for the retag pass to recognize these reborrows? Like, are they marked as syntactic sugar somewhere, or so? I recall we do that for some constructs, but I am not actually sure how it is represented.

Long-term I hope we can change SB to make raw retags true NOPs, but that will require a bit more work.

Hmm, no, the read I added to the array case has FakeReadCause::ForIndex, but nothing for the slice case.

I guess if I was to mark the &raw const as something it'd be in the SourceInfo that I'd do that, by marking its span somehow?

Alternatively, AFAIK this is always dealing in a place where the projection chain starts with a deref, like (*_1).3. So is there a way in MIR building to emit just a PtrMetadata(copy _1) that always works? The problem I hit is that when _1 is a &mut [_], that blows up later in MIR borrowck, thus why I added the &raw const in the first place.

I'm afraid I don't know much about MIR borrowck.

I guess if I was to mark the &raw const as something it'd be in the SourceInfo that I'd do that, by marking its span somehow?

Yeah I think that's how the desugaring markers usually work, but I am not sure.

davidtwco

LGTM, r=me if the issue with the Miri test is resolved

rustbot · 2024-12-13T10:00:30Z

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

scottmcm · 2024-12-13T10:02:03Z

compiler/rustc_const_eval/src/interpret/step.rs

-                if !place_base_raw {
+                if !place_base_raw
+                    && span.desugaring_kind() != Some(DesugaringKind::IndexBoundsCheckReborrow)
+                {
                    // If this was not already raw, it needs retagging.
+                    // Unless it's the `PtrMetadata(&raw const *_n)` from indexing.
                    val = M::retag_ptr_value(self, mir::RetagKind::Raw, &val)?;
                }


Please take a look at this update, @RalfJung -- I was able to revert the miri test change with it, but I have no idea if this is the right place to actually check something like this.

As mentioned before, please add a slice index expression to mir-opt/retag.rs so one can see what the final MIR looks like.

Oh never mind, I forgot that we don't have these retags in MIR any more.

Yeah this is kind of a hack but it works, thanks!

compiler/rustc_const_eval/src/interpret/step.rs

Co-authored-by: Ralf Jung <post@ralfj.de>

scottmcm · 2024-12-14T00:44:08Z

@bors r=davidtwco,RalfJung

bors · 2024-12-14T00:44:11Z

📌 Commit d2a98f7 has been approved by davidtwco,RalfJung

It is now in the queue for this repository.

…data, r=davidtwco,RalfJung Bounds-check with PtrMetadata instead of Len in MIR Rather than emitting `Len(*_n)` in array index bounds checks, emit `PtrMetadata(copy _n)` instead -- with some asterisks for arrays and `&mut` that need it to be done slightly differently. We're getting pretty close to removing `Len` entirely, actually. I think just one more PR after this (for slice drop shims). r? mir

matthiaskrgr · 2024-12-14T15:58:42Z

@bors rollup=never
for perf

bors · 2024-12-14T22:43:43Z

⌛ Testing commit d2a98f7 with merge b57d93d...

bors · 2024-12-15T01:23:33Z

☀️ Test successful - checks-actions
Approved by: davidtwco,RalfJung
Pushing b57d93d to master...

rust-timer · 2024-12-15T02:44:41Z

Finished benchmarking commit (b57d93d): comparison URL.

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

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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
Regressions ❌ (secondary)	0.3%	[0.2%, 0.3%]	2
Improvements ✅ (primary)	-1.4%	[-1.4%, -1.4%]	1
Improvements ✅ (secondary)	-0.6%	[-0.8%, -0.4%]	6
All ❌✅ (primary)	-1.4%	[-1.4%, -1.4%]	1

Max RSS (memory usage)

Results (primary 3.2%, secondary -0.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)	6.0%	[4.9%, 6.6%]	3
Regressions ❌ (secondary)	1.8%	[1.5%, 2.2%]	2
Improvements ✅ (primary)	-5.1%	[-5.1%, -5.1%]	1
Improvements ✅ (secondary)	-3.9%	[-3.9%, -3.9%]	1
All ❌✅ (primary)	3.2%	[-5.1%, 6.6%]	4

Cycles

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

Binary size

Results (primary -0.0%, secondary 0.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.1%	[0.0%, 0.5%]	14
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-0.2%	[-1.0%, -0.0%]	12
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-1.0%, 0.5%]	26

Bootstrap: 768.736s -> 772.313s (0.47%)
Artifact size: 333.17 MiB -> 333.13 MiB (-0.01%)

Kobzol · 2024-12-23T14:06:06Z

Single tiny regression, otherwise tiny improvements.

@rustbot label: +perf-regression-triaged

compiler-errors · 2025-01-18T04:10:51Z

compiler/rustc_span/src/hygiene.rs

@@ -1163,6 +1163,9 @@ pub enum DesugaringKind {
    WhileLoop,
    /// `async Fn()` bound modifier
    BoundModifier,
+    /// Marks a `&raw const *_1` needed as part of getting the length of a mutable
+    /// slice for the bounds check, so that MIRI's retag handling can recognize it.
+    IndexBoundsCheckReborrow,


Span should definitely not be used like this. Span desugarings should only ever be used for diagnostics and things like that -- making miri rely on this behavior seems really bad. If we need to track certain locals for the purposes of some special miri hack, this needs to be recorded in the MIR body separately.

cc @scottmcm @RalfJung

It's a pretty bad hack, yeah. Medium-term I hope that Miri can entirely stop doing retagging on &raw, but this currently runs into issues... so not sure if it's worth spending a lot of effort on the proper thing.

compiler-errors · 2025-01-18T04:31:40Z

compiler/rustc_middle/src/mir/consts.rs

@@ -463,6 +463,9 @@ impl<'tcx> Const<'tcx> {
                let const_val = tcx.valtree_to_const_val((ty, valtree));
                Self::Val(const_val, ty)
            }
+            ty::ConstKind::Unevaluated(ty::UnevaluatedConst { def, args }) => {


This is not correct, and there's no justification here? What is this for?

Specifically, there's a distinction between ty::ConstKind::Unevaluated(ty::UnevaluatedConst(..)) and mir::ConstKind::Unevaluated(..) and I think this may be getting that wrong. But generally randomly changing a function to do something different without asking why it's that way that seems like it may introduce subtle bugs 🤔

…trmetadata, r=davidtwco,RalfJung" This reverts commit b57d93d, reversing changes made to 0aeaa5e.

Temporarily bring back `Rvalue::Len` r? `@compiler-errors` as requested in rust-lang#135671 (comment) > However, in the mean time, I'd rather we not crunch trying to find and more importantly validate the soundness of a solution 🤔 Agreed. To fix the IMO P-critical rust-lang#135671 for which we somehow didn't have test coverage, this PR temporarily reverts: - rust-lang#133734 - its bugfix rust-lang#134371 - rust-lang#134330 cc `@scottmcm` I added the few samples from that issue as a test, but we can add more in the future, in particular it seems `@steffahn` [will work on that](rust-lang#135671 (comment)). Fixes rust-lang#135671. And if we want to land this, it should also be nominated for beta backport.

…ptrmetadata, r=davidtwco,RalfJung" This reverts commit 122a55b.

rustbot assigned davidtwco Dec 2, 2024

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 Dec 2, 2024

scottmcm commented Dec 2, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

scottmcm closed this Dec 2, 2024

scottmcm reopened this Dec 2, 2024

scottmcm force-pushed the lower-indexing-to-ptrmetadata branch 2 times, most recently from 2471745 to a7f9bf7 Compare December 2, 2024 11:06

This comment has been minimized.

Sign in to view

scottmcm force-pushed the lower-indexing-to-ptrmetadata branch from a7f9bf7 to 90b3895 Compare December 3, 2024 18:53

Bounds-check with PtrMetadata instead of Len in MIR

612adbb

scottmcm force-pushed the lower-indexing-to-ptrmetadata branch from 90b3895 to 612adbb Compare December 3, 2024 19:06

scottmcm commented Dec 3, 2024

View reviewed changes

davidtwco approved these changes Dec 12, 2024

View reviewed changes

Don't retag the PtrMetadata(&raw const *_n) in slice indexing

38249be

scottmcm commented Dec 13, 2024

View reviewed changes

RalfJung reviewed Dec 13, 2024

View reviewed changes

compiler/rustc_const_eval/src/interpret/step.rs Outdated Show resolved Hide resolved

Update compiler/rustc_const_eval/src/interpret/step.rs

d2a98f7

Co-authored-by: Ralf Jung <post@ralfj.de>

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2024

Zalathar mentioned this pull request Dec 14, 2024

Rollup of 10 pull requests #134289

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 15, 2024

bors merged commit b57d93d into rust-lang:master Dec 15, 2024
7 checks passed

rustbot added this to the 1.85.0 milestone Dec 15, 2024

rustbot added the perf-regression Performance regression. label Dec 15, 2024

scottmcm mentioned this pull request Dec 15, 2024

Delete Rvalue::Len 🎉 #134330

Merged

matthiaskrgr mentioned this pull request Dec 15, 2024

ICE: invalid asymmetric binary op Lt #134352

Open

rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 23, 2024

lqd mentioned this pull request Dec 31, 2024

[ICE]: nightly rustc panic on invalid code #134961

Closed

4 tasks

compiler-errors mentioned this pull request Jan 18, 2025

regression: cannot borrow ... as immutable because it is also borrowed as mutable #135671

Open

compiler-errors reviewed Jan 18, 2025

View reviewed changes

lqd added a commit to lqd/rust that referenced this pull request Jan 18, 2025

Revert "Auto merge of rust-lang#133734 - scottmcm:lower-indexing-to-p…

6634e6b

…trmetadata, r=davidtwco,RalfJung" This reverts commit b57d93d, reversing changes made to 0aeaa5e.

lqd added a commit to lqd/rust that referenced this pull request Jan 18, 2025

Revert "Auto merge of rust-lang#133734 - scottmcm:lower-indexing-to-p…

122a55b

…trmetadata, r=davidtwco,RalfJung" This reverts commit b57d93d, reversing changes made to 0aeaa5e.

lqd mentioned this pull request Jan 18, 2025

Temporarily bring back Rvalue::Len #135709

Merged

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jan 19, 2025

Reapply "Auto merge of rust-lang#133734 - scottmcm:lower-indexing-to-…

d9291d0

…ptrmetadata, r=davidtwco,RalfJung" This reverts commit 122a55b.

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jan 20, 2025

Reapply "Auto merge of rust-lang#133734 - scottmcm:lower-indexing-to-…

2f2593f

…ptrmetadata, r=davidtwco,RalfJung" This reverts commit 122a55b.

compiler-errors mentioned this pull request Jan 20, 2025

Lower index bounds checking to PtrMetadata, this time with the right fake borrow semantics 😸 #135748

Open

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jan 20, 2025

Reapply "Auto merge of rust-lang#133734 - scottmcm:lower-indexing-to-…

c5b76e5

…ptrmetadata, r=davidtwco,RalfJung" This reverts commit 122a55b.

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jan 21, 2025

Reapply "Auto merge of rust-lang#133734 - scottmcm:lower-indexing-to-…

119020f

…ptrmetadata, r=davidtwco,RalfJung" This reverts commit 122a55b.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bounds-check with PtrMetadata instead of Len in MIR #133734

Bounds-check with PtrMetadata instead of Len in MIR #133734

scottmcm commented Dec 2, 2024

rustbot commented Dec 2, 2024

scottmcm Dec 2, 2024

scottmcm Dec 2, 2024

This comment has been minimized.

scottmcm commented Dec 2, 2024 •

edited

Loading

This comment has been minimized.

rustbot commented Dec 3, 2024

scottmcm Dec 3, 2024

RalfJung Dec 4, 2024 •

edited

Loading

RalfJung Dec 4, 2024

scottmcm Dec 4, 2024

RalfJung Dec 4, 2024

scottmcm Dec 5, 2024

RalfJung Dec 5, 2024

RalfJung Dec 5, 2024

davidtwco left a comment

rustbot commented Dec 13, 2024

scottmcm Dec 13, 2024 •

edited

Loading

RalfJung Dec 13, 2024

RalfJung Dec 13, 2024

RalfJung Dec 13, 2024

scottmcm commented Dec 14, 2024

bors commented Dec 14, 2024

matthiaskrgr commented Dec 14, 2024

bors commented Dec 14, 2024

bors commented Dec 15, 2024

rust-timer commented Dec 15, 2024

Kobzol commented Dec 23, 2024

compiler-errors Jan 18, 2025 •

edited

Loading

RalfJung Jan 18, 2025 •

edited

Loading

compiler-errors Jan 18, 2025

compiler-errors Jan 18, 2025

		//~[stack]^ ERROR: /write access .* tag does not exist in the borrow stack/
		//~[stack]^ ERROR: /trying to retag .+ for SharedReadOnly permission .+ tag does not exist in the borrow stack for this location/

Bounds-check with PtrMetadata instead of Len in MIR #133734

Bounds-check with PtrMetadata instead of Len in MIR #133734

Conversation

scottmcm commented Dec 2, 2024

rustbot commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

scottmcm commented Dec 2, 2024 • edited Loading

This comment has been minimized.

rustbot commented Dec 3, 2024

Choose a reason for hiding this comment

RalfJung Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidtwco left a comment

Choose a reason for hiding this comment

rustbot commented Dec 13, 2024

scottmcm Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Dec 14, 2024

bors commented Dec 14, 2024

matthiaskrgr commented Dec 14, 2024

bors commented Dec 14, 2024

bors commented Dec 15, 2024

rust-timer commented Dec 15, 2024

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

Kobzol commented Dec 23, 2024

compiler-errors Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

RalfJung Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Dec 2, 2024 •

edited

Loading

RalfJung Dec 4, 2024 •

edited

Loading

scottmcm Dec 13, 2024 •

edited

Loading

compiler-errors Jan 18, 2025 •

edited

Loading

RalfJung Jan 18, 2025 •

edited

Loading