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

Delete Rvalue::Len 🎉 #134330

Merged
merged 2 commits into from
Dec 22, 2024
Merged

Delete Rvalue::Len 🎉 #134330

merged 2 commits into from
Dec 22, 2024

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Dec 15, 2024 •
edited
Loading

Everything's moved to PtrMetadata, so we can get rid of the Len variant now.

Depends on #134326, so draft until that lands Ready!

r? mir

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 15, 2024
scottmcm
scottmcm commented Dec 15, 2024
View reviewed changes
compiler/rustc_mir_build/src/build/matches/test.rs Outdated
// actual = len(place)
self.cfg.push_assign(block, source_info, actual, Rvalue::Len(place));
let actual = self.len_of_slice_or_array(block, place, test.span, source_info);
Copy link
Member Author

Choose a reason for hiding this comment

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

One functional change here, reusing this helper from #133734

scottmcm
scottmcm commented Dec 15, 2024
View reviewed changes
tests/mir-opt/building/custom/arrays.rs
mir! {
{
let x = [5_i32; C];
let c = Len(x);
Copy link
Member Author

Choose a reason for hiding this comment

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

This was here testing the custom-mir Len, which is removed here, so I deleted the test.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Dec 15, 2024

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

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@scottmcm
Delete Rvalue::Len
5ba54c9 
Everything's moved to `PtrMetadata` instead.
@scottmcm
Update clippy
29889fd
@scottmcm scottmcm closed this Dec 22, 2024
@scottmcm scottmcm reopened this Dec 22, 2024
@scottmcm scottmcm marked this pull request as ready for review December 22, 2024 16:27
@rustbot
Copy link
Collaborator

rustbot commented Dec 22, 2024

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @rust-lang/wg-const-eval

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@matthewjasper
Copy link
Contributor

Discussion of this was here for anyone wondering: https://rust-lang.zulipchat.com/#narrow/channel/189540-t-compiler.2Fwg-mir-opt/topic/.60RValue.3A.3ALen.60.20vs.20.60UnOp.3A.3APtrMetadata.60

@bors r+ rollup=iffy
(iffy for perf)

@bors
Copy link
Contributor

bors commented Dec 22, 2024

📌 Commit 29889fd has been approved by matthewjasper

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 Dec 22, 2024
@bors
Copy link
Contributor

bors commented Dec 22, 2024

⌛ Testing commit 29889fd with merge e108481...

@bors
Copy link
Contributor

bors commented Dec 22, 2024

☀️ Test successful - checks-actions
Approved by: matthewjasper
Pushing e108481 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 22, 2024
@bors bors merged commit e108481 into rust-lang:master Dec 22, 2024
13 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 22, 2024
@scottmcm scottmcm deleted the no-more-rvalue-len branch December 22, 2024 21:57
@bors bors mentioned this pull request Dec 22, 2024
Open
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e108481): 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.3% [0.3%, 0.4%] 2
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.3% [-0.8%, -0.2%] 7
Improvements ✅
(secondary)		 -0.3% [-0.4%, -0.2%] 2
All ❌✅ (primary) -0.2% [-0.8%, 0.4%] 9

Max RSS (memory usage)

Results (primary 1.1%, secondary -1.5%)

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)		 8.3% [6.8%, 9.9%] 2
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -3.7% [-5.4%, -2.3%] 3
Improvements ✅
(secondary)		 -1.5% [-1.5%, -1.5%] 1
All ❌✅ (primary) 1.1% [-5.4%, 9.9%] 5

Cycles

Results (primary 4.5%, secondary 3.6%)

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

Binary size

Results (primary 0.1%, secondary 0.0%)

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.2% [0.1%, 0.8%] 11
Regressions ❌
(secondary)		 0.0% [0.0%, 0.0%] 1
Improvements ✅
(primary)		 -0.1% [-0.4%, -0.0%] 8
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.1% [-0.4%, 0.8%] 19

Bootstrap: 765.312s -> 763.813s (-0.20%)
Artifact size: 330.59 MiB -> 330.68 MiB (0.03%)

@rustbot rustbot added the perf-regression Performance regression. label Dec 22, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Dec 23, 2024

More improvements than regressions.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 23, 2024
remi-delmas-3000 pushed a commit to remi-delmas-3000/kani that referenced this pull request Jan 6, 2025
Propagate removal of RValue::Len(place) in MIR.
05e6550 
Propagated form rust-lang/rust#134330.
`RValue::Len(place)` is still present in StableMIR,
so we translate it back to MIR as `PtrMetadata(Copy(place))`.
remi-delmas-3000 pushed a commit to remi-delmas-3000/kani that referenced this pull request Jan 7, 2025
Propagate removal of RValue::Len(place) in MIR.
99c3f62 
Propagated form rust-lang/rust#134330.
`RValue::Len(place)` is still present in StableMIR,
so we translate it back to MIR as `PtrMetadata(Copy(place))`.
remi-delmas-3000 pushed a commit to remi-delmas-3000/kani that referenced this pull request Jan 7, 2025
Propagate removal of RValue::Len(place) in MIR.
8159275 
Propagated form rust-lang/rust#134330.
`RValue::Len(place)` is still present in StableMIR,
so we translate it back to MIR as `PtrMetadata(Copy(place))`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@matthewjasper matthewjasper

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.85.0
Development

Successfully merging this pull request may close these issues.
7 participants
@scottmcm @rust-log-analyzer @bors @rustbot @matthewjasper @rust-timer @Kobzol