interpret: track place alignment together with the type, not the value by RalfJung · Pull Request #98846 · rust-lang/rust

RalfJung · 2022-07-03T14:26:14Z

This matches how I handle alignment in MiniRust. I think it makes conceptually a lot more sense.
Fixes #63085

rustbot · 2022-07-03T14:26:17Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung · 2022-07-03T14:26:44Z

This makes MemPlace and Place a bit smaller. Unfortunately it also makes OpTy a bit bigger. Let's see if that is a problem.
@bors try @rust-timer queue

rust-timer · 2022-07-03T14:26:45Z

Awaiting bors try build completion.

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

bors · 2022-07-03T14:26:51Z

⌛ Trying commit 8955686 with merge 66f86800cc3af8ccdffbe1b0ab2f547612df788d...

bors · 2022-07-03T15:58:42Z

☀️ Try build successful - checks-actions
Build commit: 66f86800cc3af8ccdffbe1b0ab2f547612df788d (66f86800cc3af8ccdffbe1b0ab2f547612df788d)

rust-timer · 2022-07-03T15:58:43Z

Queued 66f86800cc3af8ccdffbe1b0ab2f547612df788d with parent f99f9e4, future comparison URL.

rust-timer · 2022-07-03T22:10:26Z

Finished benchmarking commit (66f86800cc3af8ccdffbe1b0ab2f547612df788d): comparison url.

Instruction count

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

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: no relevant changes found

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

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)	-3.6%	-3.6%	1
Improvements 🎉 (secondary)	-3.8%	-3.8%	1
All 😿🎉 (primary)	-3.6%	-3.6%	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-04T09:26:27Z

+    /// This means `layout.align` should never be used for an `OpTy`!
+    /// `None` means "alignment does not matter since this is a by-value operand"
+    /// (`Operand::Immediate`).
+    pub align: Option<Align>,


So afaict this field is never used, and after lots of going through this PRs diff, I came to the conclusion it will only be used to check reads from Operand::Indirect, and only in miri.

Please document the miri part, the other part is already documented after all.

Yeah currently this field cannot cause UB I think. But I want to relax the rules for *ptr and once we do, this can cause UB again.

oli-obk · 2022-07-04T14:53:13Z

@bors r+

bors · 2022-07-04T14:53:15Z

📌 Commit b1568e6 has been approved by oli-obk

bors · 2022-07-05T01:23:12Z

⌛ Testing commit b1568e6 with merge 4008dd8...

bors · 2022-07-05T04:03:59Z

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

rust-timer · 2022-07-05T10:59:20Z

Finished benchmarking commit (4008dd8): 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.7%	-0.9%	8
Improvements 🎉 (secondary)	-1.2%	-2.3%	13
All 😿🎉 (primary)	-0.7%	-0.9%	8

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	3.0%	6.2%	3
Regressions 😿 (secondary)	2.8%	3.8%	6
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	3.0%	6.2%	3

Cycles

Results

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

	mean¹	max	count²
Regressions 😿 (primary)	2.7%	3.0%	2
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.5%	-2.8%	3
Improvements 🎉 (secondary)	-3.4%	-4.6%	7
All 😿🎉 (primary)	-0.4%	3.0%	5

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

RalfJung · 2022-07-05T11:30:33Z

Ah, those Max-RSS regressions are unfortunate. I have some ideas for how to make OpTy smaller again.

RalfJung · 2022-07-06T12:51:16Z

Okay, that plan actually works. But it is the 4th PR in a linear chain starting with #98831 ...^^

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

8955686

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

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

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

oli-obk reviewed Jul 4, 2022

View reviewed changes

clarify comment

b1568e6

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

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

bors merged commit 4008dd8 into rust-lang:master Jul 5, 2022

rustbot added this to the 1.64.0 milestone Jul 5, 2022

This was referenced Jul 5, 2022

Use niche-filling optimization even when multiple variants have data. #94075

Merged

Use only ty::Unevaluated<'tcx, ()> in type system #98588

Merged

RalfJung deleted the alignment-is-a-type-thing branch July 5, 2022 11:30

RalfJung mentioned this pull request Jul 9, 2022

shrink OpTy back down to 80 bytes #99097

Closed

Uh oh!

Conversation

RalfJung commented Jul 3, 2022

Uh oh!

rustbot commented Jul 3, 2022

Uh oh!

RalfJung commented Jul 3, 2022

Uh oh!

rust-timer commented Jul 3, 2022

Uh oh!

bors commented Jul 3, 2022

Uh oh!

bors commented Jul 3, 2022

Uh oh!

rust-timer commented Jul 3, 2022

Uh oh!

rust-timer commented Jul 3, 2022

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Jul 4, 2022

Uh oh!

bors commented Jul 4, 2022

Uh oh!

bors commented Jul 5, 2022

Uh oh!

bors commented Jul 5, 2022

Uh oh!

rust-timer commented Jul 5, 2022

Footnotes

Uh oh!

RalfJung commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RalfJung commented Jul 5, 2022 •

edited

Loading

RalfJung commented Jul 6, 2022 •

edited

Loading