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.

Sign up for GitHub

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

Jump to bottom

some ConstValue refactoring #115764

Merged
merged 7 commits into from Sep 14, 2023
Merged

some ConstValue refactoring #115764

merged 7 commits into from Sep 14, 2023

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 11, 2023 •
edited

In particular, use AllocId instead of Allocation in ConstValue::ByRef. This helps avoid redundant AllocIds when a ByRef constant gets put back into the interpreter.

r? @oli-obk

Fixes #105536

@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 Sep 11, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

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

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@RalfJung
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 11, 2023
@bors
Copy link
Contributor

bors commented Sep 11, 2023

⌛ Trying commit 50ad68a with merge 993d146...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2023
@bors
Auto merge of rust-lang#115764 - RalfJung:const-by-ref-alloc-id, r=<try>
993d146 
use AllocId instead of Allocation in ConstValue::ByRef

This helps avoid redundant AllocIds when a  `ByRef` constant gets put back into the interpreter.

r? `@oli-obk`
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

Ah I guess we don't want to print these ByRef allocations -- at least currently one couldn't even tell where they are referenced in the code, I think.

@bors
Copy link
Contributor

bors commented Sep 11, 2023

☀️ Try build successful - checks-actions
Build commit: 993d146 (993d146a8d653e110488abd05a676643e7e14d04)

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Sep 12, 2023

The Miri subtree was changed

cc @rust-lang/miri

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (993d146): comparison URL.

Overall result: no relevant changes - no action needed

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-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

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)		 2.7% [2.7%, 2.7%] 1
Regressions ❌
(secondary)		 3.0% [2.3%, 4.2%] 3
Improvements ✅
(primary)		 -0.7% [-0.7%, -0.7%] 1
Improvements ✅
(secondary)		 -3.8% [-5.2%, -2.3%] 4
All ❌✅ (primary) 1.0% [-0.7%, 2.7%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 629.503s -> 630.23s (0.12%)
Artifact size: 317.74 MiB -> 317.81 MiB (0.02%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 12, 2023
@RalfJung
Copy link
Member Author

Okay, so doing it just for ByRef makes no difference. Now for the full version including slices.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 12, 2023
@bors
Copy link
Contributor

bors commented Sep 12, 2023

⌛ Trying commit 3163962 with merge b3d0b96...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2023
@bors
Auto merge of rust-lang#115764 - RalfJung:const-by-ref-alloc-id, r=<try>
b3d0b96 
use AllocId instead of Allocation in ConstValue::ByRef

This helps avoid redundant AllocIds when a  `ByRef` constant gets put back into the interpreter.

r? `@oli-obk`
@rust-log-analyzer

This comment has been minimized.

Sign in to view
RalfJung
RalfJung commented Sep 12, 2023
View reviewed changes
compiler/rustc_const_eval/src/const_eval/eval_queries.rs Outdated
let len: usize = len.try_into().unwrap();
ConstValue::Slice { data, start, end: start + len }
ConstValue::Slice { alloc_id, start, end: start + len }
Copy link
Member Author

@RalfJung RalfJung Sep 12, 2023 •
edited

Choose a reason for hiding this comment

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

Uh, I just realized that valtree-to-const-value will hit this code path for all wide pointers, e.g. &[u32]. That doesn't seem right? Certainly we didn't write the start/end logic thinking about anything besides u8/str...

Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably generalize ConstValue::Slice to be used for arbitrary types that have a slice as their unsized tail. That justifies how valtrees are using them in valtree-to-constval conversion.

But that's for a future PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still hoping to be able to revive my ConstValue::Slice removal PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, with the simplification of op_to_const in this PR, the Slice variant isn't so bad any more. In particular these days it seems to be okay to have slices in ConstValue::ByRef, they don't have to be in ConstValue::Slice, and then it's pretty okay.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's leave fixing the valtree problem to the next PR; this one is ready now.

@bors
Copy link
Contributor

bors commented Sep 12, 2023

☀️ Try build successful - checks-actions
Build commit: b3d0b96 (b3d0b9649ed03fa422d630948a874e0861f55286)

@rust-timer

This comment has been minimized.

Sign in to view
@RalfJung RalfJung changed the title use AllocId instead of Allocation in ConstValue::ByRef some ConstValue refactoring Sep 12, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor

oli-obk commented Sep 14, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 14, 2023

📌 Commit 6a1c887 has been approved by oli-obk

It is now in the queue for this repository.

@bors 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 Sep 14, 2023
This was referenced Sep 14, 2023
Closed
Closed
@oli-obk
Copy link
Contributor

oli-obk commented Sep 14, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 14, 2023

📌 Commit ed41c1e has been approved by oli-obk

It is now in the queue for this repository.

@RalfJung
Copy link
Member Author

Ah, I was working on a lengthy comment to better explain what happens in op_to_const. Pushed that now.

I think I'm finally almost happy with the state of that function. :) it only took like 4 years or so.^^
("almost" due to the issue with valtree slices, I'll tackle that next.)

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Sep 14, 2023

📌 Commit 053173d has been approved by oli-obk

It is now in the queue for this repository.

@RalfJung
Copy link
Member Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Sep 14, 2023

📌 Commit 04a4df5 has been approved by oli-obk

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 14, 2023

⌛ Testing commit 04a4df5 with merge d97e04f...

@bors
Copy link
Contributor

bors commented Sep 14, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 14, 2023
@bors bors merged commit d97e04f into rust-lang:master Sep 14, 2023
12 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 14, 2023
@RalfJung RalfJung deleted the const-by-ref-alloc-id branch September 14, 2023 14:33
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d97e04f): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.1% [-1.1%, -1.1%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

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)		 3.9% [3.9%, 3.9%] 1
Regressions ❌
(secondary)		 2.6% [2.5%, 2.8%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.5% [-5.1%, -0.9%] 6
All ❌✅ (primary) 3.9% [3.9%, 3.9%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 632.387s -> 631.221s (-0.18%)
Artifact size: 317.97 MiB -> 317.98 MiB (0.00%)

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 15, 2023
@bors
Auto merge of #115803 - RalfJung:const-eval, r=oli-obk
3448551 
make the eval() functions on our const types return the resulting value

This is a part of rust-lang/rust#115748 that's hopefully perf-neutral, and that does not depend on rust-lang/rust#115764.
@RalfJung RalfJung mentioned this pull request Oct 13, 2023
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@oli-obk oli-obk

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.

slice::from_raw_parts returns a different address in const context for u8
7 participants
@RalfJung @rustbot @rust-timer @bors @rust-log-analyzer @oli-obk @bjorn3