interpret: get rid of MemPlaceMeta::Poison #99013

RalfJung · 2022-07-07T12:27:06Z

This is achieved by refactoring the projection code ({mplace,place,operand}_{downcast,field,index,...}) so that we no longer need to call assert_mem_place in the operand handling.

rustbot · 2022-07-07T12:27:10Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in const_evaluatable.rs

cc @lcnr

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

rust-highfive · 2022-07-07T12:27:10Z

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

compiler/rustc_const_eval/src/interpret/projection.rs

RalfJung · 2022-07-07T12:38:56Z

compiler/rustc_const_eval/src/interpret/place.rs

@@ -275,12 +273,12 @@ impl<'tcx, Tag: Provenance> OpTy<'tcx, Tag> {
            Operand::Indirect(mplace) => {
                Ok(MPlaceTy { mplace, layout: self.layout, align: self.align.unwrap() })
            }
-            Operand::Immediate(_) if self.layout.is_zst() => Ok(MPlaceTy::dangling(self.layout)),


This was the main motivation for poisoning: we'd come up with a place (that has an address) for operands that don't have an address.

All other uses of MPlaceTy::dangling are fine, they are actually conceptually properly creating an allocation. I was looking for good ways to reflect that in its name and failed at that. MPlaceTy::alloc_zst sounds like it actually creates an allocation. Maybe fake_alloc_zst or so?

Maybe fake_alloc_zst or so?

I like this idea. dangling has a lot of meaning attached to it that doesn't immediately convey what we mean here.

All right, done that.

RalfJung · 2022-07-07T12:40:10Z

r? @oli-obk

Let's see if those new assertions cost us any perf (and we need something clever trait-based).
@bors try @rust-timer queue

bors · 2022-07-13T05:30:10Z

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

MPlaceTy::dangling still exists, but now it is only called in places that actually conceptually allocate something new, so that's fine.

RalfJung · 2022-07-13T14:28:21Z

Also ready for review now. :)

And now for our swarm of bots...
@rustbot ready
@bors try
@rust-timer queue

rust-timer · 2022-07-13T14:28:23Z

Awaiting bors try build completion.

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

bors · 2022-07-13T14:28:29Z

⌛ Trying commit 874a130 with merge 430074764e7eb1cee93eb99022271bca8aee5140...

rust-timer · 2022-07-13T14:29:53Z

Awaiting bors try build completion.

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

rust-timer · 2022-07-13T14:30:31Z

Awaiting bors try build completion.

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

rust-timer · 2022-07-13T14:30:51Z

Awaiting bors try build completion.

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

bors · 2022-07-13T16:32:44Z

☀️ Try build successful - checks-actions
Build commit: 430074764e7eb1cee93eb99022271bca8aee5140 (430074764e7eb1cee93eb99022271bca8aee5140)

rust-timer · 2022-07-13T16:32:45Z

Queued 430074764e7eb1cee93eb99022271bca8aee5140 with parent ca4e394, future comparison URL.

rust-timer · 2022-07-13T19:50:25Z

Finished benchmarking commit (430074764e7eb1cee93eb99022271bca8aee5140): 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)	-1.0%	-1.2%	7
All 😿🎉 (primary)	N/A	N/A	0

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)	-9.1%	-9.1%	1
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: mixed results

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

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-14T16:04:58Z

@bors r+

bors · 2022-07-14T16:04:59Z

📌 Commit e3ef4fd has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-07-15T08:58:08Z

⌛ Testing commit e3ef4fd with merge 6077b7c...

bors · 2022-07-15T11:44:11Z

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

rust-timer · 2022-07-15T13:01:03Z

Finished benchmarking commit (6077b7c): 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)	0.5%	0.5%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-1.6%	-2.5%	12
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	4.1%	4.6%	2
Regressions 😿 (secondary)	2.2%	2.2%	1
Improvements 🎉 (primary)	-0.6%	-0.6%	1
Improvements 🎉 (secondary)	-6.0%	-8.4%	2
All 😿🎉 (primary)	2.6%	4.6%	3

Cycles

Results

Primary benchmarks: 🎉 relevant improvement 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)	-2.7%	-2.7%	1
Improvements 🎉 (secondary)	-2.1%	-2.1%	1
All 😿🎉 (primary)	-2.7%	-2.7%	1

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 ↩ ↩² ↩³

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

rust-highfive assigned jackh726 Jul 7, 2022

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

RalfJung commented Jul 7, 2022

View reviewed changes

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

RalfJung commented Jul 7, 2022

View reviewed changes

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

RalfJung commented Jul 7, 2022

View reviewed changes

RalfJung force-pushed the dont-poison-my-places branch from 7b38d51 to a1dc2af Compare July 7, 2022 12:40

rust-highfive assigned oli-obk Jul 7, 2022

This comment was marked as outdated.

Sign in to view

rust-highfive unassigned jackh726 Jul 7, 2022

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

This comment was marked as outdated.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 8, 2022

RalfJung force-pushed the dont-poison-my-places branch from a1dc2af to 2b76d0e Compare July 8, 2022 11:39

This comment was marked as outdated.

Sign in to view

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

This comment was marked as outdated.

Sign in to view

This comment has been minimized.

Sign in to view

RalfJung force-pushed the dont-poison-my-places branch from 2b76d0e to 09bd020 Compare July 8, 2022 11:48

get rid of MemPlaceMeta::Poison

874a130

MPlaceTy::dangling still exists, but now it is only called in places that actually conceptually allocate something new, so that's fine.

RalfJung force-pushed the dont-poison-my-places branch from ab9e2f3 to 874a130 Compare July 13, 2022 14:27

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

rename MPlaceTy::dangling to fake_alloc_zst

e3ef4fd

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 14, 2022

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

bors merged commit 6077b7c into rust-lang:master Jul 15, 2022

rustbot added this to the 1.64.0 milestone Jul 15, 2022

RalfJung deleted the dont-poison-my-places branch July 17, 2022 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interpret: get rid of MemPlaceMeta::Poison #99013

interpret: get rid of MemPlaceMeta::Poison #99013

RalfJung commented Jul 7, 2022 •

edited

rustbot commented Jul 7, 2022

rust-highfive commented Jul 7, 2022

RalfJung Jul 7, 2022

oli-obk Jul 14, 2022

RalfJung Jul 14, 2022

RalfJung commented Jul 7, 2022

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment has been minimized.

bors commented Jul 13, 2022

RalfJung commented Jul 13, 2022

rust-timer commented Jul 13, 2022

bors commented Jul 13, 2022

rust-timer commented Jul 13, 2022

rust-timer commented Jul 13, 2022

rust-timer commented Jul 13, 2022

bors commented Jul 13, 2022

rust-timer commented Jul 13, 2022

rust-timer commented Jul 13, 2022

oli-obk commented Jul 14, 2022

bors commented Jul 14, 2022

bors commented Jul 15, 2022

bors commented Jul 15, 2022

rust-timer commented Jul 15, 2022

interpret: get rid of MemPlaceMeta::Poison #99013

interpret: get rid of MemPlaceMeta::Poison #99013

Conversation

RalfJung commented Jul 7, 2022 • edited

rustbot commented Jul 7, 2022

rust-highfive commented Jul 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jul 7, 2022

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment has been minimized.

bors commented Jul 13, 2022

RalfJung commented Jul 13, 2022

rust-timer commented Jul 13, 2022

bors commented Jul 13, 2022

rust-timer commented Jul 13, 2022

rust-timer commented Jul 13, 2022

rust-timer commented Jul 13, 2022

bors commented Jul 13, 2022

rust-timer commented Jul 13, 2022

rust-timer commented Jul 13, 2022

Footnotes

oli-obk commented Jul 14, 2022

bors commented Jul 14, 2022

bors commented Jul 15, 2022

bors commented Jul 15, 2022

rust-timer commented Jul 15, 2022

Footnotes

RalfJung commented Jul 7, 2022 •

edited