interpret: remove support for unsized_locals #98831

RalfJung · 2022-07-03T01:17:06Z

I added support for unsized_locals in #59780 but the current implementation is a crude hack and IMO definitely not the right way to have unsized locals in MIR. It also causes problems. and what codegen does is unsound and has been for years since clearly nobody cares (so I hope nobody actually relies on that implementation and I'll be happy if Miri ensures they do not). I think if we want to have unsized locals in Miri/MIR we should add them properly, either by having a StorageLive that takes metadata or by having an alloca that returns a pointer (making the ptr indirection explicit) or something like that.

So, this PR removes the LocalValue::Unallocated hack. It adds Immediate::Uninit, for several reasons:

This lets us still do fairly little work in push_stack_frame, in particular we do not actually have to create any allocations.
If/when I remove ScalarMaybeUninit, we will need something like this to have an "optimized" representation of uninitialized locals. Without this we'd have to put uninitialized integers into the heap!
const-prop needs some way to indicate "I don't know the value of this local'; it used to use LocalValue::Unallocated for that, now it can use Immediate::Uninit.

There is still a fundamental difference between LocalValue::Unallocated and Immediate::Uninit: the latter is considered a regular local that you can read from and write to, it just has a more optimized representation when compared with an actual Allocation that is fully uninit. In contrast, LocalValue::Unallocated had this really odd behavior where you would write to it but not read from it. (This is in fact what caused the problems mentioned above.)

While at it I also did two drive-by cleanups/improvements:

In pop_stack_frame, do the return value copying and local deallocation while the frame is still on the stack. This leads to better error locations being reported. The old errors were sometimes rather confusing.
Deduplicate copy_op and copy_op_transmute.

r? @oli-obk

rustbot · 2022-07-03T01:17:10Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler/rustc_const_eval/src/interpret/eval_context.rs

oli-obk · 2022-07-04T11:33:48Z

@bors try @rust-timer queue

rust-timer · 2022-07-04T11:33:50Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-07-04T11:33:57Z

⌛ Trying commit 52625e2cda587d5d4111a762e96c597f6a61a561 with merge 82ac962b53ec83a970c2f76fb4c21c590a27e6d5...

bors · 2022-07-04T13:06:43Z

☀️ Try build successful - checks-actions
Build commit: 82ac962b53ec83a970c2f76fb4c21c590a27e6d5 (82ac962b53ec83a970c2f76fb4c21c590a27e6d5)

rust-timer · 2022-07-04T13:06:45Z

Queued 82ac962b53ec83a970c2f76fb4c21c590a27e6d5 with parent 9c9ae85, future comparison URL.

rust-timer · 2022-07-05T03:18:27Z

Finished benchmarking commit (82ac962b53ec83a970c2f76fb4c21c590a27e6d5): comparison url.

Instruction count

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-0.6%	-0.9%	13
Improvements 🎉 (secondary)	-1.0%	-2.3%	23
All 😿🎉 (primary)	-0.6%	-0.9%	13

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.5%	-2.5%	1
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	3.2%	3.2%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-3.2%	-3.9%	6
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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.

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

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

oli-obk · 2022-07-05T07:57:27Z

r=me after a rebase

RalfJung · 2022-07-05T11:37:38Z

Oh, bors never said there was a conflict, interesting...

RalfJung · 2022-07-05T12:31:28Z

@bors r=oli-obk

bors · 2022-07-05T12:31:29Z

📌 Commit 6b927a50903e3d4243480c968a7ddd68882d08dc has been approved by oli-obk

bors · 2022-07-06T17:38:42Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout no-more-unsized-locals (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self no-more-unsized-locals --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging src/test/ui/consts/offset_from_ub.stderr
Auto-merging compiler/rustc_mir_transform/src/const_prop_lint.rs
Auto-merging compiler/rustc_mir_transform/src/const_prop.rs
Auto-merging compiler/rustc_const_eval/src/interpret/step.rs
Auto-merging compiler/rustc_const_eval/src/interpret/place.rs
CONFLICT (content): Merge conflict in compiler/rustc_const_eval/src/interpret/place.rs
Auto-merging compiler/rustc_const_eval/src/interpret/operand.rs
Auto-merging compiler/rustc_const_eval/src/interpret/machine.rs
Auto-merging compiler/rustc_const_eval/src/interpret/intrinsics.rs
Auto-merging compiler/rustc_const_eval/src/interpret/cast.rs
Automatic merge failed; fix conflicts and then commit the result.

Operand::Uninit is an *allocated* operand that is fully uninitialized. This lets us lazily allocate the actual backing store of *all* locals (no matter their ABI). I also reordered things in pop_stack_frame at the same time. I should probably have made that a separate commit...

RalfJung · 2022-07-06T18:12:18Z

@bors r=oli-obk

bors · 2022-07-06T18:12:20Z

📌 Commit dc9e0bf has been approved by oli-obk

bors · 2022-07-06T22:50:32Z

⌛ Testing commit dc9e0bf with merge 8824d13...

bors · 2022-07-07T01:17:30Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 8824d13 to master...

adjust for removed unsized_locals The Miri side of rust-lang/rust#98831

rust-timer · 2022-07-07T14:26:01Z

Finished benchmarking commit (8824d13): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-0.6%	-0.8%	11
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	0.1%	0.1%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-1.7%	-1.7%	1
All 😿🎉 (primary)	0.1%	0.1%	1

Cycles

Results

Primary benchmarks: mixed results
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	2.3%	2.3%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.6%	-2.6%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-0.1%	-2.6%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

…oli-obk don't allow ZST in ScalarInt There are several indications that we should not ZST as a ScalarInt: - We had two ways to have ZST valtrees, either an empty `Branch` or a `Leaf` with a ZST in it. `ValTree::zst()` used the former, but the latter could possibly arise as well. - Likewise, the interpreter had `Immediate::Uninit` and `Immediate::Scalar(Scalar::ZST)`. - LLVM codegen already had to special-case ZST ScalarInt. So I propose we stop using ScalarInt to represent ZST (which are clearly not integers). Instead, we can add new ZST variants to those types that did not have other variants which could be used for this purpose. Based on rust-lang#98831. Only the commits starting from "don't allow ZST in ScalarInt" are new. r? `@oli-obk`

…i-obk don't allow ZST in ScalarInt There are several indications that we should not ZST as a ScalarInt: - We had two ways to have ZST valtrees, either an empty `Branch` or a `Leaf` with a ZST in it. `ValTree::zst()` used the former, but the latter could possibly arise as well. - Likewise, the interpreter had `Immediate::Uninit` and `Immediate::Scalar(Scalar::ZST)`. - LLVM codegen already had to special-case ZST ScalarInt. So I propose we stop using ScalarInt to represent ZST (which are clearly not integers). Instead, we can add new ZST variants to those types that did not have other variants which could be used for this purpose. Based on rust-lang#98831. Only the commits starting from "don't allow ZST in ScalarInt" are new. r? `@oli-obk`

…-obk Re-enable removal of ZST writes to unions This was previously disabled because Miri was lazily allocating unsized locals. But we aren't doing that anymore since rust-lang#98831, so we can have this optimization back.

Re-enable removal of ZST writes to unions This was previously disabled because Miri was lazily allocating unsized locals. But we aren't doing that anymore since rust-lang/rust#98831, so we can have this optimization back.

rust-highfive assigned oli-obk Jul 3, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 3, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2022

RalfJung mentioned this pull request Jul 3, 2022

adjust for removed unsized_locals rust-lang/miri#2314

Merged

RalfJung force-pushed the no-more-unsized-locals branch from f1cc003 to b007399 Compare July 3, 2022 01:48

This comment has been minimized.

Sign in to view

bjorn3 reviewed Jul 3, 2022

View reviewed changes

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

RalfJung force-pushed the no-more-unsized-locals branch from 7356614 to 70fbc06 Compare July 3, 2022 11:51

This comment was marked as resolved.

Sign in to view

RalfJung force-pushed the no-more-unsized-locals branch from 70fbc06 to 52625e2 Compare July 3, 2022 12:55

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 4, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 5, 2022

RalfJung force-pushed the no-more-unsized-locals branch from 52625e2 to 6b927a5 Compare July 5, 2022 11:54

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 Jul 5, 2022

This was referenced Jul 5, 2022

don't allow ZST in ScalarInt #98957

Merged

Rollup of 5 pull requests #98970

Merged

interpret: track place alignment together with the type, not the value #98846

Merged

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 6, 2022

RalfJung added 5 commits July 6, 2022 14:03

support passing unsized fn arguments

47cb276

bless

924a4cd

deduplicate some copy_op code

e685530

fix a strange ConstProp ICE

dc9e0bf

RalfJung force-pushed the no-more-unsized-locals branch from 6b927a5 to dc9e0bf Compare July 6, 2022 18:11

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 6, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 7, 2022

bors merged commit 8824d13 into rust-lang:master Jul 7, 2022

rustbot added this to the 1.64.0 milestone Jul 7, 2022

bors added a commit to rust-lang/miri that referenced this pull request Jul 7, 2022

Auto merge of #2314 - RalfJung:unsized, r=RalfJung

399b481

adjust for removed unsized_locals The Miri side of rust-lang/rust#98831

bors added a commit to rust-lang/miri that referenced this pull request Jul 7, 2022

Auto merge of #2314 - RalfJung:unsized, r=RalfJung

968b032

adjust for removed unsized_locals The Miri side of rust-lang/rust#98831

bors added a commit to rust-lang/miri that referenced this pull request Jul 7, 2022

Auto merge of #2314 - RalfJung:unsized, r=RalfJung

64dec22

adjust for removed unsized_locals The Miri side of rust-lang/rust#98831

RalfJung deleted the no-more-unsized-locals branch July 8, 2022 00:51

saethlin mentioned this pull request Dec 3, 2022

Re-enable removal of ZST writes to unions #105229

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interpret: remove support for unsized_locals #98831

interpret: remove support for unsized_locals #98831

RalfJung commented Jul 3, 2022

rustbot commented Jul 3, 2022

This comment has been minimized.

This comment was marked as resolved.

oli-obk commented Jul 4, 2022

rust-timer commented Jul 4, 2022

bors commented Jul 4, 2022

bors commented Jul 4, 2022

rust-timer commented Jul 4, 2022

rust-timer commented Jul 5, 2022

oli-obk commented Jul 5, 2022

RalfJung commented Jul 5, 2022

RalfJung commented Jul 5, 2022

bors commented Jul 5, 2022

bors commented Jul 6, 2022

RalfJung commented Jul 6, 2022

bors commented Jul 6, 2022

bors commented Jul 6, 2022

bors commented Jul 7, 2022

rust-timer commented Jul 7, 2022

interpret: remove support for unsized_locals #98831

interpret: remove support for unsized_locals #98831

Conversation

RalfJung commented Jul 3, 2022

rustbot commented Jul 3, 2022

This comment has been minimized.

This comment was marked as resolved.

oli-obk commented Jul 4, 2022

rust-timer commented Jul 4, 2022

bors commented Jul 4, 2022

bors commented Jul 4, 2022

rust-timer commented Jul 4, 2022

rust-timer commented Jul 5, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

oli-obk commented Jul 5, 2022

RalfJung commented Jul 5, 2022

RalfJung commented Jul 5, 2022

bors commented Jul 5, 2022

bors commented Jul 6, 2022

RalfJung commented Jul 6, 2022

bors commented Jul 6, 2022

bors commented Jul 6, 2022

bors commented Jul 7, 2022

rust-timer commented Jul 7, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes